Re: [PATCH 04/22] clk: mediatek: Add MT8195 basic clocks support | linux-mediatek

(off-list ancestor, not in this archive)

On Thu, 2021-07-22 at 15:44 +0800, Chen-Yu Tsai wrote:
Hi,

It seems your reply included HTML, which means that it never reached
the mailing lists. Please always use plaintext only.

On Thu, Jul 22, 2021 at 08:17:40AM +0800, Chun-Jie Chen wrote:
On Fri, 2021-07-02 at 19:44 +0800, Chen-Yu Tsai wrote:
On Thu, Jun 17, 2021 at 7:05 AM Chun-Jie Chen
[off-list ref] wrote:
Add MT8195 basic clock providers, include topckgen,
apmixedsys,
infracfg_ao and pericfg_ao.

Signed-off-by: Chun-Jie Chen <redacted>
---
 drivers/clk/mediatek/Kconfig      |    8 +
 drivers/clk/mediatek/Makefile     |    1 +
 drivers/clk/mediatek/clk-mt8195.c | 1958
+++++++++++++++++++++++++++++
 3 files changed, 1967 insertions(+)
 create mode 100644 drivers/clk/mediatek/clk-mt8195.c

diff --git a/drivers/clk/mediatek/Kconfig
b/drivers/clk/mediatek/Kconfig
index 576babd86f98..6707aba3d500 100644
--- a/drivers/clk/mediatek/Kconfig
+++ b/drivers/clk/mediatek/Kconfig
@@ -580,6 +580,14 @@ config COMMON_CLK_MT8192_VENCSYS
        help
          This driver supports MediaTek MT8192 vencsys
clocks.

+config COMMON_CLK_MT8195
+       bool "Clock driver for MediaTek MT8195"
+       depends on ARM64 || COMPILE_TEST
+       select COMMON_CLK_MEDIATEK
+       default ARM64
+       help
+         This driver supports MediaTek MT8195 basic
clocks.
+
 config COMMON_CLK_MT8516
        bool "Clock driver for MediaTek MT8516"
        depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/clk/mediatek/Makefile
b/drivers/clk/mediatek/Makefile
index 15bc045f0b71..f8002d8966e1 100644
--- a/drivers/clk/mediatek/Makefile
+++ b/drivers/clk/mediatek/Makefile
@@ -80,5 +80,6 @@ obj-$(CONFIG_COMMON_CLK_MT8192_MSDC) +=
clk-
mt8192-msdc.o
 obj-$(CONFIG_COMMON_CLK_MT8192_SCP_ADSP) +=
clk-mt8192-scp_adsp.o
 obj-$(CONFIG_COMMON_CLK_MT8192_VDECSYS) += clk-mt8192-
vdec.o
 obj-$(CONFIG_COMMON_CLK_MT8192_VENCSYS) += clk-mt8192-
venc.o
+obj-$(CONFIG_COMMON_CLK_MT8195) += clk-mt8195.o
 obj-$(CONFIG_COMMON_CLK_MT8516) += clk-mt8516.o
 obj-$(CONFIG_COMMON_CLK_MT8516_AUDSYS) += clk-mt8516-aud.o
diff --git a/drivers/clk/mediatek/clk-mt8195.c
b/drivers/clk/mediatek/clk-mt8195.c
new file mode 100644
index 000000000000..aea9ebe4c051
--- /dev/null
+++ b/drivers/clk/mediatek/clk-mt8195.c
@@ -0,0 +1,1958 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (c) 2021 MediaTek Inc.
+// Author: Chun-Jie Chen <chun-jie.chen@mediatek.com>
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "clk-mtk.h"
+#include "clk-mux.h"
+#include "clk-gate.h"
+
+#include <dt-bindings/clock/mt8195-clk.h>
+
+static DEFINE_SPINLOCK(mt8195_clk_lock);
+
+static const struct mtk_fixed_clk top_fixed_clks[] = {
+       FIXED_CLK(CLK_TOP_IN_DGI, "in_dgi", NULL,
165000000),
+       FIXED_CLK(CLK_TOP_ULPOSC, "ulposc", NULL,
248000000),
+       FIXED_CLK(CLK_TOP_ULPOSC2, "ulposc2", NULL,
326000000),
+       FIXED_CLK(CLK_TOP_MEM_466M, "mem_466m", NULL,
533000000),
+       FIXED_CLK(CLK_TOP_MPHONE_SLAVE_B, "mphone_slave_b",
NULL,
49152000),
+       FIXED_CLK(CLK_TOP_PEXTP_PIPE, "pextp_pipe", NULL,
250000000),
+       FIXED_CLK(CLK_TOP_UFS_RX_SYMBOL, "ufs_rx_symbol",
NULL,
166000000),
+       FIXED_CLK(CLK_TOP_UFS_TX_SYMBOL, "ufs_tx_symbol",
NULL,
166000000),
+       FIXED_CLK(CLK_TOP_SSUSB_U3PHY_P1_P_P0,
"ssusb_u3phy_p1_p_p0", NULL, 131000000),
+       FIXED_CLK(CLK_TOP_UFS_RX_SYMBOL1, "ufs_rx_symbol1",
NULL,
166000000),
+       FIXED_CLK(CLK_TOP_FPC, "fpc", NULL, 50000000),
+       FIXED_CLK(CLK_TOP_HDMIRX_P, "hdmirx_p", NULL,
594000000),

I assume these are fixed PLLs? They should have inputs
(parents).

Moreover, at least ULPOSC and ULPOSC2 look like they are in
APMIXEDSYS
The clock in top_fixed_clks is special clock that generated from
the
specific hardware block, not PLLs in APMIXEDSYS. ULPOSC and ULPOSC2
has
configuration register in APMIXEDSYS, but their clock source are
not
"clk26m" (other plls in APMIXEDSYS in is generated from "clk26m")
I see. Surely they have some input though. It would be nice to be
able
to have them described.

+};T
+
+static const struct mtk_fixed_factor top_early_divs[] = {
+       FACTOR(CLK_TOP_CLK26M_D2, "clk26m_d2", "clk26m", 1,
2),
+};
+
+static const struct mtk_fixed_factor top_divs[] = {
+       FACTOR(CLK_TOP_CLK26M_D52, "clk26m_d52", "clk26m",
1,
52),
+       FACTOR(CLK_TOP_IN_DGI_D2, "in_dgi_d2", "in_dgi", 1,
2),
+       FACTOR(CLK_TOP_IN_DGI_D4, "in_dgi_d4", "in_dgi", 1,
4),
+       FACTOR(CLK_TOP_IN_DGI_D6, "in_dgi_d6", "in_dgi", 1,
6),
+       FACTOR(CLK_TOP_IN_DGI_D8, "in_dgi_d8", "in_dgi", 1,
8),
+       FACTOR(CLK_TOP_MFGPLL_OPP, "mfgpll_opp", "mfgpll",
1,
1),
+       FACTOR(CLK_TOP_MAINPLL, "mainpll_ck", "mainpll", 1,
1),
Why are this and other 1:1 factor clks needed? They look like
placeholders.
Please remove them.


These 1:1 factors make more readable between dividers. For example,
CLK_APMIXED_MAINPLL and CLK_TOP_MAINPLL_D3 is not easy to see the
relation, but CLK_TOP_MAINPLL and CLK_TOP_MAINPLL_D3 is more clear
to
see the relation.
If the clocks are named appropriately, it should be clear that
"mainpll_dX"
is derived from "mainpll". We really don't need an extra "mainpll_ck"
in
between.

The only thing gained here is having the parent clock in the same
driver.
But that is only a problem because we are directly using global clock
names
for parent names. This isn't the preferred way for clock parenting.

For proper parenting, the driver should be using `struct
clk_parent_data`
if possible, or using of_clk_get_parent_name() or of_clk_get_hw()
manually
to get the parent's global name or a reference to it. This is
something
the clk drivers should slowly be converted to doing.

I'm not saying we should do everything now, but we can start by
getting
rid of some of the excess baggage.

[...]
Ok, I will remove the 1-to-1 divider in next version.
Thanks for your comment.


+static const char * const dsp7_parents[] = {
+       "clk26m",
+       "univpll_d6_d2",
+       "univpll_d4_d2",
+       "univpll_d5",
+       "univpll_d4",
+       "mmpll_d4",
+       "mainpll_d3",
+       "univpll_d3"
+};
If dsp~dsp7_parents are all the same, please merge them and
share
one
instance. And since they are located a bit far from the clock
definitions
in this file, please add comments describing which clocks share
the
same
set of parents.
I will merge it if they can share the same parent source data
(include
you mention below), thanks for your comment.
Great!

[...]

Tip: You can trim out portions of the original email from your reply,
like
what I did here, so that the emails are shorter. Keeping just the
bits that
are relevant to the discussion is better for the reader. In cases
here a
lot of it are related cases, you could keep just the one nearest to
your
reply.

+static const char * const spinor_parents[] = {
+       "clk26m",
Datasheet says first parent is "univpll_d5_d8". Please check
with
hardware
engineers. If the datasheet is wrong please add a comment
saying so.

+       "clk26m_d2",
+       "mainpll_d7_d8",
+       "univpll_d6_d8"
+};
+
The parent source here is correct, but not update to the latest in
datasheet.
For future reference, could you leave a comment stating that the
datasheet
has not been updated then? Please include the version of the
datasheet.
The parent source here was updated at v25 internal version.

+static const char * const dvio_dgi_ref_parents[] = {
+       "clk26m",
+       "in_dgi",
+       "in_dgi_d2",
+       "in_dgi_d4",
+       "in_dgi_d6",
+       "in_dgi_d8",
+       "mmpll_d4_d4"
+};
+
+static const char * const srck_parents[] = {
+       "ulposc_d10",
+       "clk26m"
+};
+
+static const char * const rsvd1_parents[] = {
+       "clk26m",
+       "mainpll_d4_d4",
+       "mainpll_d5_d4",
+       "mainpll_d6_d4",
+       "mainpll_d7_d4",
+       "univpll_d6_d4",
These are completely different from the datasheet. Please
check.

+       "ulposc",
+       "ulposc2"
+};
+
The parent source here is correct, but not update to the latest in
datasheet.
Same for this one.
The parent source here was updated at v25 internal version.

+static const char * const mfg_fast_parents[] = {
+       "mfg_sel",
+       "mfgpll_opp"
+};
+
+static const struct mtk_mux top_mtk_muxes[] = {
+       /* CLK_CFG_0 */
+       MUX_GATE_CLR_SET_UPD_FLAGS(CLK_TOP_AXI_SEL,
"axi_sel",
Please drop the "_sel" suffix from the clock names. It would
have
made
sense if this were purely a mux, and there was another clock
gate.
But
since the driver combines the two components into one
representation,
please just drop the suffix that implies just a mux. This goes
for
all
clocks in the series and also the macro bindings.


If we move the "_sel" suffix from clock names, it's hard to
represent
this mux with gate control. Do you think revise it to
"XXX_sel_gate" in
CCF name but keep the binding name because change the binding name
need
all CCF consumer changes.
Please elaborate. Does the "type" of the clock matter? All that is
really needed is that the name is unique, matches the datasheet more
or
less, and describes the usage or purpose of the clock.

For example, on Allwinner sunxi platforms, we don't include the type
of the clock in the clock names. Only the base clock name is used.
That is because the clocks are modeled as composite-ish clocks, so
only one clock is needed to describe a full mux+divider+gate.

On other platforms, the clock driver deliberately uses base clock
types,
mux, div, and gate, to build up a representation of the full clock
unit.
In these cases, we end up with "XXX_mux", "XXX_div", and "XXX_gate".

Since the Mediatek clock driver is more like the first case, I would
prefer to see clock names with just the base name, and none of the
typing.

And regarding binding names, please change them as well. Right now
the
only place that needs to be changed are the header files. This is the
time to get them right.
Ok, I will remove suffix "_SEL" in binding name and change
clock name from "xxx_sel" to "top_xxx".
Thanks for your comment.

+               axi_parents, 0x020, 0x024, 0x028, 0, 3, 7,
0x04,
0,
CLK_IS_CRITICAL),
+       MUX_GATE_CLR_SET_UPD_FLAGS(CLK_TOP_SPM_SEL,
"spm_sel",
+               spm_parents, 0x020, 0x024, 0x028, 8, 2, 15,
0x04,
1, CLK_IS_CRITICAL),
Where is the SCP clock?


Because SCP is always on and no other clock gates need to reference
it,
so move it.
Please add a comment as a placeholder then. The comment could simply
state "clock is always on and should not be handled by Linux".

[...]
Ok, I will leave some comment to describe why skip or why make is
critical in next version.

+       /* CLK_CFG_11 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_USB_SEL, "usb_sel",
The datasheet lists these as "usb_top" and "usb_top_Xp". Please
keep
the name
the same as the datasheet so it is easy to search for. Also
note the
discrepency
between the macro name and the clock name. Same goes for the
three
other USB
clocks.


Do you think revise the name the same as datasheet in CCF name but
keep
binding name?
Please have them match each other. The whole point of keeping names
consistent is to be able to search for them easily.
Ok, I will revise the clock name which can match in datasheet.


+               usb_parents, 0x0A4, 0x0A8, 0x0AC, 0, 2, 7,
0x08,
12),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SSUSB_XHCI_SEL,
"ssusb_xhci_sel",
+               ssusb_xhci_parents, 0x0A4, 0x0A8, 0x0AC, 8,
2,
15,
0x08, 13),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_USB_1P_SEL,
"usb_1p_sel",
+               usb_1p_parents, 0x0A4, 0x0A8, 0x0AC, 16, 2,
23,
0x08, 14),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SSUSB_XHCI_1P_SEL,
"ssusb_xhci_1p_sel",
+               ssusb_xhci_1p_parents, 0x0A4, 0x0A8, 0x0AC,
24,
2,
31, 0x08, 15),
+       /* CLK_CFG_12 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_USB_2P_SEL,
"usb_2p_sel",
+               usb_2p_parents, 0x0B0, 0x0B4, 0x0B8, 0, 2,
7,
0x08,
16),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SSUSB_XHCI_2P_SEL,
"ssusb_xhci_2p_sel",
+               ssusb_xhci_2p_parents, 0x0B0, 0x0B4, 0x0B8,
8,
2,
15, 0x08, 17),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_USB_3P_SEL,
"usb_3p_sel",
+               usb_3p_parents, 0x0B0, 0x0B4, 0x0B8, 16, 2,
23,
0x08, 18),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SSUSB_XHCI_3P_SEL,
"ssusb_xhci_3p_sel",
+               ssusb_xhci_3p_parents, 0x0B0, 0x0B4, 0x0B8,
24,
2,
31, 0x08, 19),
+       /* CLK_CFG_13 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_I2C_SEL, "i2c_sel",
+               i2c_parents, 0x0BC, 0x0C0, 0x0C4, 0, 2, 7,
0x08,
20),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SENINF_SEL,
"seninf_sel",
+               seninf_parents, 0x0BC, 0x0C0, 0x0C4, 8, 3,
15,
0x08, 21),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SENINF1_SEL,
"seninf1_sel",
+               seninf1_parents, 0x0BC, 0x0C0, 0x0C4, 16,
3, 23,
0x08, 22),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SENINF2_SEL,
"seninf2_sel",
+               seninf2_parents, 0x0BC, 0x0C0, 0x0C4, 24,
3, 31,
0x08, 23),
+       /* CLK_CFG_14 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SENINF3_SEL,
"seninf3_sel",
+               seninf3_parents, 0x0C8, 0x0CC, 0x0D0, 0, 3,
7,
0x08, 24),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_GCPU_SEL, "gcpu_sel",
+               gcpu_parents, 0x0C8, 0x0CC, 0x0D0, 8, 3,
15,
0x08,
25),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_DXCC_SEL, "dxcc_sel",
+               dxcc_parents, 0x0C8, 0x0CC, 0x0D0, 16, 2,
23,
0x08,
26),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_DPMAIF_SEL,
"dpmaif_sel",
+               dpmaif_parents, 0x0C8, 0x0CC, 0x0D0, 24, 3,
31,
0x08, 27),
+       /* CLK_CFG_15 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_AES_UFSFDE_SEL,
"aes_ufsfde_sel",
+               aes_ufsfde_parents, 0x0D4, 0x0D8, 0x0DC, 0,
3,
7,
0x08, 28),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_UFS_SEL, "ufs_sel",
+               ufs_parents, 0x0D4, 0x0D8, 0x0DC, 8, 3, 15,
0x08,
29),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_UFS_TICK1US_SEL,
"ufs_tick1us_sel",
+               ufs_tick1us_parents, 0x0D4, 0x0D8, 0x0DC,
16, 1,
23, 0x08, 30),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_UFS_MP_SAP_SEL,
"ufs_mp_sap_sel",
+               ufs_mp_sap_parents, 0x0D4, 0x0D8, 0x0DC,
24, 1,
31,
0x08, 31),
+       /* CLK_CFG_16 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_VENC_SEL, "venc_sel",
+               venc_parents, 0x0E0, 0x0E4, 0x0E8, 0, 4, 7,
0x0C,
0),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_VDEC_SEL, "vdec_sel",
+               vdec_parents, 0x0E0, 0x0E4, 0x0E8, 8, 4,
15,
0x0C,
1),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_PWM_SEL, "pwm_sel",
+               pwm_parents, 0x0E0, 0x0E4, 0x0E8, 16, 1,
23,
0x0C,
2),
MCU clock? Not sure what it's supposed to be called since the
naming
has a
slightly different format.

If you are skipping clocks, please leave a comment in the list
explaining
why.
"mcupm" is another micro-processor and is always on, and no
others
clock gates need to reference it, so remove it.
Please add a comment, just like in the SCP clock case.

+       /* CLK_CFG_17 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SPMI_P_MST_SEL,
"spmi_p_mst_sel",
+               spmi_p_mst_parents, 0x0EC, 0x0F0, 0x0F4, 0,
4,
7,
0x0C, 4),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SPMI_M_MST_SEL,
"spmi_m_mst_sel",
+               spmi_m_mst_parents, 0x0EC, 0x0F0, 0x0F4, 8,
4,
15,
0x0C, 5),
DVFSRC clock?
"DVFSRC" IP block is always on  and no others clock gates need to
reference it, so remove it.
This one as well.

[...]

+       /* CLK_CFG_24 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_APLL5_SEL,
"apll5_sel",
+               apll5_parents, 0x0140, 0x0144, 0x0148, 0,
1, 7,
0x010, 0),
For the APLLs, you will need to differentiate them from the
actual
PLLs in
the APMIXEDSYS block.
APLLX providers more precise clock so it has more configuration
register, but it still has same control flow like other PLLs in
APMIXEDSYS.
I think what I meant was, because I asked to remove the "_sel"
suffix,
the global clock names here now collide with the ones in APMIXEDSYS.
So what I was asking was the names here need to be changed, maybe to
something like "top_apllX".

+       MUX_GATE_CLR_SET_UPD(CLK_TOP_I2SO1_M_SEL,
"i2so1_m_sel",
+               i2so1_m_parents, 0x0140, 0x0144, 0x0148, 8,
3,
15,
0x010, 1),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_I2SO2_M_SEL,
"i2so2_m_sel",
+               i2so2_m_parents, 0x0140, 0x0144, 0x0148,
16, 3,
23,
0x010, 2),
I2SO4_M?

+       /* CLK_CFG_25 */
I2SO5_M?

+       MUX_GATE_CLR_SET_UPD(CLK_TOP_I2SI1_M_SEL,
"i2si1_m_sel",
+               i2si1_m_parents, 0x014C, 0x0150, 0x0154, 8,
3,
15,
0x010, 5),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_I2SI2_M_SEL,
"i2si2_m_sel",
+               i2si2_m_parents, 0x014C, 0x0150, 0x0154,
16, 3,
23,
0x010, 6),
I2SI4_M?

+       /* CLK_CFG_26 */
I2SI5_M?
I2SO4/I2SO5/I2SI4_M/I2SI5_M is not used in MT8195, so remove it
here.
Please add comments as placeholders for them. The comment could state
that the clock is unused in the hardware design, so it was skipped.

+       MUX_GATE_CLR_SET_UPD(CLK_TOP_DPTX_M_SEL,
"dptx_m_sel",
+               dptx_m_parents, 0x0158, 0x015C, 0x0160, 8,
3,
15,
0x010, 9),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_AUD_IEC_SEL,
"aud_iec_sel",
+               aud_iec_parents, 0x0158, 0x015C, 0x0160,
16, 3,
23,
0x010, 10),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_A1SYS_HP_SEL,
"a1sys_hp_sel",
+               a1sys_hp_parents, 0x0158, 0x015C, 0x0160,
24, 1,
31, 0x010, 11),
+       /* CLK_CFG_27 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_A2SYS_SEL,
"a2sys_sel",
+               a2sys_parents, 0x0164, 0x0168, 0x016C, 0,
1, 7,
0x010, 12),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_A3SYS_SEL,
"a3sys_sel",
+               a3sys_parents, 0x0164, 0x0168, 0x016C, 8,
3, 15,
0x010, 13),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_A4SYS_SEL,
"a4sys_sel",
+               a4sys_parents, 0x0164, 0x0168, 0x016C, 16,
3,
23,
0x010, 14),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SPINFI_B_SEL,
"spinfi_b_sel",
+               spinfi_b_parents, 0x0164, 0x0168, 0x016C,
24, 3,
31, 0x010, 15),
+       /* CLK_CFG_28 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_NFI1X_SEL,
"nfi1x_sel",
+               nfi1x_parents, 0x0170, 0x0174, 0x0178, 0,
3, 7,
0x010, 16),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_ECC_SEL, "ecc_sel",
+               ecc_parents, 0x0170, 0x0174, 0x0178, 8, 3,
15,
0x010, 17),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_AUDIO_LOCAL_BUS_SEL,
"audio_local_bus_sel",
+               audio_local_bus_parents, 0x0170, 0x0174,
0x0178,
16, 4, 23, 0x010, 18),
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_SPINOR_SEL,
"spinor_sel",
+               spinor_parents, 0x0170, 0x0174, 0x0178, 24,
2,
31,
0x010, 19),
+       /* CLK_CFG_29 */
+       MUX_GATE_CLR_SET_UPD(CLK_TOP_DVIO_DGI_REF_SEL,
"dvio_dgi_ref_sel",
+               dvio_dgi_ref_parents, 0x017C, 0x0180,
0x0184, 0,
3,
7, 0x010, 20),
ULPOSC and ULPOSC_CORE?


ULPOSC and ULPOSC_CORE is always on and no other clock gate needs
to
reference it, so just remove it.
Please add placeholder comments describing them, specifically why
they
are missing from the driver. Are the clocks used at all in the
hardware
design? If so then it might be better to add them to the clock
driver.

Also, even if they, including SCP and MCU above, are always on, it
might
still be nice to have driver support for them, just to be able to
read
their state from Linux. We could have CLK_IS_CRITICAL set so they
don't
get disabled, and CLK_GET_RATE_NOCACHE if the clocks are expected to
be
changed by firmware running alongside of Linux.

+       MUX_GATE_CLR_SET_UPD_FLAGS(CLK_TOP_SRCK_SEL,
"srck_sel",
+               srck_parents, 0x017C, 0x0180, 0x0184, 24,
1, 31,
0x010, 23, CLK_IS_CRITICAL),
What happened to CLK_CFG_30~36?
The muxes in CLK_CFG_30 ~ 36 are not used, so just remove it from
CCF.
Please add placeholder comments about them.

+       /* CLK_CFG_37 */
+       MUX_GATE_CLR_SET_UPD_FLAGS(CLK_TOP_RSVD1_SEL,
"rsvd1_sel",
+               rsvd1_parents, 0x01DC, 0x01E0, 0x01E4, 0,
3, 7,
0x014, 20, CLK_IS_CRITICAL),
What about the other three?

"rsvd2" and "rsvd3" is not used, so remove it.
Same here.

To be honest, a "rsvd", which I interpret as "reserved", clock being
used seems kind of contradicting. Do we know what hardware is using
rsvd1?

+};
+
+static struct mtk_composite top_muxes[] = {
+       /* CLK_MISC_CFG_3 */
+       MUX(CLK_TOP_MFG_FAST_SEL, "mfg_fast_sel",
mfg_fast_parents,
0x0250, 8, 1),
+};
+
+static const struct mtk_composite top_adj_divs[] = {
+       DIV_GATE(CLK_TOP_APLL12_DIV0, "apll12_div0",
"i2si1_m_sel",
0x0320, 0, 0x0328, 8, 0),
+       DIV_GATE(CLK_TOP_APLL12_DIV1, "apll12_div1",
"i2si2_m_sel",
0x0320, 1, 0x0328, 8, 8),
+       DIV_GATE(CLK_TOP_APLL12_DIV2, "apll12_div2",
"i2so1_m_sel",
0x0320, 2, 0x0328, 8, 16),
+       DIV_GATE(CLK_TOP_APLL12_DIV3, "apll12_div3",
"i2so2_m_sel",
0x0320, 3, 0x0328, 8, 24),
+       DIV_GATE(CLK_TOP_APLL12_DIV4, "apll12_div4",
"aud_iec_sel",
0x0320, 4, 0x0334, 8, 0),
What about 5~8?
div5 ~ 8 are not used in MT8195, so remove it.
Placeholder comments please.

[...]

+static const struct mtk_gate_regs top0_cg_regs = {
+       .set_ofs = 0x238,
+       .clr_ofs = 0x238,
+       .sta_ofs = 0x238,
+};
+
+static const struct mtk_gate_regs top1_cg_regs = {
+       .set_ofs = 0x250,
+       .clr_ofs = 0x250,
+       .sta_ofs = 0x250,
+};
+
+#define GATE_TOP0_FLAGS(_id, _name, _parent, _shift,
_flag)            \
+       GATE_MTK_FLAGS(_id, _name, _parent, &top0_cg_regs,
_shift,      \
+               &mtk_clk_gate_ops_no_setclr_inv, _flag)
+
+#define GATE_TOP0(_id, _name, _parent,
_shift)                 \
+       GATE_TOP0_FLAGS(_id, _name, _parent, _shift, 0)
+
+#define GATE_TOP1(_id, _name, _parent,
_shift)                 \
+       GATE_MTK(_id, _name, _parent, &top1_cg_regs,
_shift,
&mtk_clk_gate_ops_no_setclr_inv)
+
+static const struct mtk_gate top_clks[] = {
+       /* TOP0 */
+       GATE_TOP0(CLK_TOP_CFG_VPP0, "cfg_vpp0", "vpp_sel",
0),
+       GATE_TOP0(CLK_TOP_CFG_VPP1, "cfg_vpp1", "vpp_sel",
1),
+       GATE_TOP0(CLK_TOP_CFG_VDO0, "cfg_vdo0", "vpp_sel",
2),
+       GATE_TOP0(CLK_TOP_CFG_VDO1, "cfg_vdo1", "vpp_sel",
3),
+       GATE_TOP0(CLK_TOP_CFG_UNIPLL_SES, "cfg_unipll_ses",
"univpll_d2", 4),
+       GATE_TOP0(CLK_TOP_CFG_26M_VPP0, "cfg_26m_vpp0",
"clk26m",
5),
+       GATE_TOP0(CLK_TOP_CFG_26M_VPP1, "cfg_26m_vpp1",
"clk26m",
6),
+       GATE_TOP0(CLK_TOP_CFG_26M_AUD, "cfg_26m_aud",
"clk26m",
9),
+       GATE_TOP0_FLAGS(CLK_TOP_CFG_AXI_EAST,
"cfg_axi_east",
"axi_sel", 10, CLK_IS_CRITICAL),
+       GATE_TOP0_FLAGS(CLK_TOP_CFG_AXI_EAST_NORTH,
"cfg_axi_east_north", "axi_sel", 11,
+               CLK_IS_CRITICAL),
+       GATE_TOP0_FLAGS(CLK_TOP_CFG_AXI_NORTH,
"cfg_axi_north",
"axi_sel", 12, CLK_IS_CRITICAL),
+       GATE_TOP0_FLAGS(CLK_TOP_CFG_AXI_SOUTH,
"cfg_axi_south",
"axi_sel", 13, CLK_IS_CRITICAL),
+       GATE_TOP0(CLK_TOP_CFG_EXT_TEST, "cfg_ext_test",
"msdcpll_d2", 15),
+       /* TOP1 */
+       GATE_TOP1(CLK_TOP_SSUSB_REF, "ssusb_ref", "clk26m",
0),
+       GATE_TOP1(CLK_TOP_SSUSB_PHY_REF, "ssusb_phy_ref",
"clk26m",
1),
+       GATE_TOP1(CLK_TOP_SSUSB_P1_REF, "ssusb_p1_ref",
"clk26m",
2),
+       GATE_TOP1(CLK_TOP_SSUSB_PHY_P1_REF,
"ssusb_phy_p1_ref",
"clk26m", 3),
+       GATE_TOP1(CLK_TOP_SSUSB_P2_REF, "ssusb_p2_ref",
"clk26m",
4),
+       GATE_TOP1(CLK_TOP_SSUSB_PHY_P2_REF,
"ssusb_phy_p2_ref",
"clk26m", 5),
+       GATE_TOP1(CLK_TOP_SSUSB_P3_REF, "ssusb_p3_ref",
"clk26m",
6),
+       GATE_TOP1(CLK_TOP_SSUSB_PHY_P3_REF,
"ssusb_phy_p3_ref",
"clk26m", 7),
+};
These should be grouped with the other TOPCKGEN clocks. Another
reason to
split this driver into multiple ones.


These clocks are "clock gate" in TOPCKGEN, so we separate the data
of
"clock gate" from "clock mux".
Right, but it gets really confusing when the driver is mixing clocks
from different clock controllers. Code that gets used together should
be grouped together. In this case, code for the same hardware block
should be grouped together. So please split the drivers up if they
end
up being really big, and then group the remaining ones by hardware
block first, then type second.
I will separate the clocks here to different clock driver, each driver
only keep the clocks that provided by same IP block.

+
+static const struct mtk_gate_regs apmixed_cg_regs = {
+       .set_ofs = 0x8,
+       .clr_ofs = 0x8,
+       .sta_ofs = 0x8,
+};
+
+#define GATE_APMIXED(_id, _name, _parent,
_shift)                      \
+       GATE_MTK(_id, _name, _parent, &apmixed_cg_regs,
_shift,
&mtk_clk_gate_ops_no_setclr_inv)
+
+static const struct mtk_gate apmixed_clks[] = {
+       GATE_APMIXED(CLK_APMIXED_PLL_SSUSB26M,
"pll_ssusb26m",
"clk26m", 1),
+};
+
+#define MT8195_PLL_FMAX                (3800UL * MHZ)
+#define MT8195_PLL_FMIN                (1500UL * MHZ)
+#define MT8195_INTEGER_BITS    8
+
+#define PLL(_id, _name, _reg, _pwr_reg, _en_mask,
_flags,      \
+                       _rst_bar_mask, _pcwbits, _pd_reg,
_pd_shift,    \
+                       _tuner_reg, _tuner_en_reg,
_tuner_en_bit,       \
+                       _pcw_reg, _pcw_shift,
_pcw_chg_reg,                             \
+                       _en_reg, _pll_en_bit)
{                                 \
+               .id =
_id,                                              \
+               .name =
_name,                                          \
+               .reg =
_reg,                                            \
+               .pwr_reg =
_pwr_reg,                                    \
+               .en_mask =
_en_mask,                                    \
+               .flags =
_flags,                                        \
+               .rst_bar_mask =
_rst_bar_mask,                          \
+               .fmax =
MT8195_PLL_FMAX,                                \
+               .fmin =
MT8195_PLL_FMIN,                                \
+               .pcwbits =
_pcwbits,                                    \
+               .pcwibits =
MT8195_INTEGER_BITS,                        \
+               .pd_reg =
_pd_reg,                                      \
+               .pd_shift =
_pd_shift,                                  \
+               .tuner_reg =
_tuner_reg,                                \
+               .tuner_en_reg =
_tuner_en_reg,                          \
+               .tuner_en_bit =
_tuner_en_bit,                          \
+               .pcw_reg =
_pcw_reg,                                    \
+               .pcw_shift =
_pcw_shift,                                \
+               .pcw_chg_reg =
_pcw_chg_reg,                            \
+               .en_reg =
_en_reg,                                      \
+               .pll_en_bit =
_pll_en_bit,                              \
+       }
+
+static const struct mtk_pll_data plls[] = {
+       PLL(CLK_APMIXED_NNAPLL, "nnapll", 0x0390, 0x03a0,
0,
+               0, 0, 22, 0x0398, 24, 0, 0, 0, 0x0398, 0,
0x0398,
0, 9),
+       PLL(CLK_APMIXED_RESPLL, "respll", 0x0190, 0x0320,
0,
+               0, 0, 22, 0x0198, 24, 0, 0, 0, 0x0198, 0,
0x0198,
0, 9),
+       PLL(CLK_APMIXED_ETHPLL, "ethpll", 0x0360, 0x0370,
0,
+               0, 0, 22, 0x0368, 24, 0, 0, 0, 0x0368, 0,
0x0368,
0, 9),
+       PLL(CLK_APMIXED_MSDCPLL, "msdcpll", 0x0710, 0x0720,
0,
+               0, 0, 22, 0x0718, 24, 0, 0, 0, 0x0718, 0,
0x0718,
0, 9),
+       PLL(CLK_APMIXED_TVDPLL1, "tvdpll1", 0x00a0, 0x00b0,
0,
+               0, 0, 22, 0x00a8, 24, 0, 0, 0, 0x00a8, 0,
0x00a8,
0, 9),
+       PLL(CLK_APMIXED_TVDPLL2, "tvdpll2", 0x00c0, 0x00d0,
0,
+               0, 0, 22, 0x00c8, 24, 0, 0, 0, 0x00c8, 0,
0x00c8,
0, 9),
+       PLL(CLK_APMIXED_MMPLL, "mmpll", 0x00e0, 0x00f0,
0xff000000,
+               HAVE_RST_BAR, BIT(23), 22, 0x00e8, 24, 0,
0, 0,
0x00e8, 0, 0x00e8, 0, 9),
+       PLL(CLK_APMIXED_MAINPLL, "mainpll", 0x01d0, 0x01e0,
0xff000000,
+               HAVE_RST_BAR, BIT(23), 22, 0x01d8, 24, 0,
0, 0,
0x01d8, 0, 0x01d8, 0, 9),
+       PLL(CLK_APMIXED_VDECPLL, "vdecpll", 0x0890, 0x08a0,
0,
+               0, 0, 22, 0x0898, 24, 0, 0, 0, 0x0898, 0,
0x0898,
0, 9),
+       PLL(CLK_APMIXED_IMGPLL, "imgpll", 0x0100, 0x0110,
0,
+               0, 0, 22, 0x0108, 24, 0, 0, 0, 0x0108, 0,
0x0108,
0, 9),
+       PLL(CLK_APMIXED_UNIVPLL, "univpll", 0x01f0, 0x0700,
0xff000000,
+               HAVE_RST_BAR, BIT(23), 22, 0x01f8, 24, 0,
0, 0,
0x01f8, 0, 0x01f8, 0, 9),
+       PLL(CLK_APMIXED_HDMIPLL1, "hdmipll1", 0x08c0,
0x08d0,
0,
+               0, 0, 22, 0x08c8, 24, 0, 0, 0, 0x08c8, 0,
0x08c8,
0, 9),
+       PLL(CLK_APMIXED_HDMIPLL2, "hdmipll2", 0x0870,
0x0880,
0,
+               0, 0, 22, 0x0878, 24, 0, 0, 0, 0x0878, 0,
0x0878,
0, 9),
+       PLL(CLK_APMIXED_HDMIRX_APLL, "hdmirx_apll", 0x08e0,
0x0dd4,
0,
+               0, 0, 32, 0x08e8, 24, 0, 0, 0, 0x08ec, 0,
0x08e8,
0, 9),
+       PLL(CLK_APMIXED_USB1PLL, "usb1pll", 0x01a0, 0x01b0,
0,
+               0, 0, 22, 0x01a8, 24, 0, 0, 0, 0x01a8, 0,
0x01a8,
0, 9),
+       PLL(CLK_APMIXED_ADSPPLL, "adsppll", 0x07e0, 0x07f0,
0,
+               0, 0, 22, 0x07e8, 24, 0, 0, 0, 0x07e8, 0,
0x07e8,
0, 9),
+       PLL(CLK_APMIXED_APLL1, "apll1", 0x07c0, 0x0dc0, 0,
+               0, 0, 32, 0x07c8, 24, 0x0470, 0x0000, 12,
0x07cc,
0, 0x07c8, 0, 9),
+       PLL(CLK_APMIXED_APLL2, "apll2", 0x0780, 0x0dc4, 0,
+               0, 0, 32, 0x0788, 24, 0x0474, 0x0000, 13,
0x078c,
0, 0x0788, 0, 9),
+       PLL(CLK_APMIXED_APLL3, "apll3", 0x0760, 0x0dc8, 0,
+               0, 0, 32, 0x0768, 24, 0x0478, 0x0000, 14,
0x076c,
0, 0x0768, 0, 9),
+       PLL(CLK_APMIXED_APLL4, "apll4", 0x0740, 0x0dcc, 0,
+               0, 0, 32, 0x0748, 24, 0x047C, 0x0000, 15,
0x074c,
0, 0x0748, 0, 9),
+       PLL(CLK_APMIXED_APLL5, "apll5", 0x07a0, 0x0dd0,
0x100000,
+               0, 0, 32, 0x07a8, 24, 0x0480, 0x0000, 16,
0x07ac,
0, 0x07a8, 0, 9),
+       PLL(CLK_APMIXED_MFGPLL, "mfgpll", 0x0340, 0x0350,
0,
+               0, 0, 22, 0x0348, 24, 0, 0, 0, 0x0348, 0,
0x0348,
0, 9),
+       PLL(CLK_APMIXED_DGIPLL, "dgipll", 0x0150, 0x0160,
0,
+               0, 0, 22, 0x0158, 24, 0, 0, 0, 0x0158, 0,
0x0158,
0, 9),
+};
+
+static struct clk_onecell_data *top_clk_data;
+
+static void clk_mt8195_top_init_early(struct device_node
*node)
+{
+       int i;
+
+       top_clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK);
+       if (!top_clk_data)
+               return;
+
+       for (i = 0; i < CLK_TOP_NR_CLK; i++)
+               top_clk_data->clks[i] = ERR_PTR(-
EPROBE_DEFER);
+
+       mtk_clk_register_factors(top_early_divs,
ARRAY_SIZE(top_early_divs), top_clk_data);
+
+       of_clk_add_provider(node, of_clk_src_onecell_get,
top_clk_data);
+}
+
+CLK_OF_DECLARE_DRIVER(mt8195_topckgen,
"mediatek,mt8195-topckgen",
+                       clk_mt8195_top_init_early);
+
+static int clk_mt8195_top_probe(struct platform_device
*pdev)
+{
+       struct device_node *node = pdev->dev.of_node;
+       int r;
+       void __iomem *base;
+
+       base = devm_platform_ioremap_resource(pdev, 0);
+       if (IS_ERR(base))
+               return PTR_ERR(base);
+
+       mtk_clk_register_fixed_clks(top_fixed_clks,
ARRAY_SIZE(top_fixed_clks),
+                       top_clk_data);
+       mtk_clk_register_factors(top_early_divs,
ARRAY_SIZE(top_early_divs), top_clk_data);
+       mtk_clk_register_factors(top_divs,
ARRAY_SIZE(top_divs),
top_clk_data);
+       mtk_clk_register_muxes(top_mtk_muxes,
ARRAY_SIZE(top_mtk_muxes), node,
+                       &mt8195_clk_lock, top_clk_data);
+       mtk_clk_register_composites(top_muxes,
ARRAY_SIZE(top_muxes), base,
+                       &mt8195_clk_lock, top_clk_data);
+       mtk_clk_register_composites(top_adj_divs,
ARRAY_SIZE(top_adj_divs), base,
+                       &mt8195_clk_lock, top_clk_data);
Future work: these functions probably should be made to return
errors.

+       r = mtk_clk_register_gates(node, top_clks,
ARRAY_SIZE(top_clks), top_clk_data);
+       if (r)
+               return r;
+
+       return of_clk_add_provider(node,
of_clk_src_onecell_get,
top_clk_data);
+}
+
+static int clk_mt8195_infra_ao_probe(struct
platform_device
*pdev)
+{
+       struct clk_onecell_data *clk_data;
+       struct device_node *node = pdev->dev.of_node;
+       int r;
+
+       clk_data = mtk_alloc_clk_data(CLK_INFRA_AO_NR_CLK);
+       if (!clk_data)
+               return -ENOMEM;
+
+       r = mtk_clk_register_gates(node, infra_ao_clks,
ARRAY_SIZE(infra_ao_clks), clk_data);
+       if (r)
+               return r;
+
+       return of_clk_add_provider(node,
of_clk_src_onecell_get,
clk_data);
You are leaking clk_data if mtk_clk_register_gates() or
of_clk_add_provider()
fail.


Ok, I will fix it include you mention below, thanks for you
comment.


+}
+
+static int clk_mt8195_apmixed_probe(struct platform_device
*pdev)
+{
+       struct clk_onecell_data *clk_data;
+       struct device_node *node = pdev->dev.of_node;
+       int r;
+
+       clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
+       if (!clk_data)
+               return -ENOMEM;
+
+       mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls),
clk_data);
+       r = mtk_clk_register_gates(node, apmixed_clks,
ARRAY_SIZE(apmixed_clks), clk_data);
+       if (r)
+               return r;
+
+       return of_clk_add_provider(node,
of_clk_src_onecell_get,
clk_data);
Same here.

+}
+
+static int clk_mt8195_peri_ao_probe(struct platform_device
*pdev)
+{
+       struct clk_onecell_data *clk_data;
+       struct device_node *node = pdev->dev.of_node;
+       int r;
+
+       clk_data = mtk_alloc_clk_data(CLK_PERI_AO_NR_CLK);
+       if (!clk_data)
+               return -ENOMEM;
+
+       r = mtk_clk_register_gates(node, peri_ao_clks,
ARRAY_SIZE(peri_ao_clks), clk_data);
+       if (r)
+               return r;
+
+       return of_clk_add_provider(node,
of_clk_src_onecell_get,
clk_data);
And here.

+}
+
+static const struct of_device_id of_match_clk_mt8195[] = {
+       {
+               .compatible = "mediatek,mt8195-apmixedsys",
+               .data = clk_mt8195_apmixed_probe,
+       }, {
+               .compatible = "mediatek,mt8195-topckgen",
+               .data = clk_mt8195_top_probe,
+       }, {
+               .compatible = "mediatek,mt8195-
infracfg_ao",
+               .data = clk_mt8195_infra_ao_probe,
+       }, {
+               .compatible = "mediatek,mt8195-pericfg_ao",
+               .data = clk_mt8195_peri_ao_probe,
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
different
files and preferably different patches so people reading them
don't
have
to look through unrelated data. Then you don't need the pointer
to
the
probe function.


Ok, I will split to different driver.


You can keep them under the same Kconfig symbol.

+       }, {
+               /* sentinel */
+       }
+};
+
+static int clk_mt8195_probe(struct platform_device *pdev)
+{
+       int (*clk_probe)(struct platform_device *pdev);
+       int r;
+
+       clk_probe = of_device_get_match_data(&pdev->dev);
+       if (!clk_probe)
+               return -EINVAL;
+
+       r = clk_probe(pdev);
+       if (r)
+               dev_err(&pdev->dev,
+                       "could not register clock provider:
%s:
%d\n",
+                       pdev->name, r);
+
+       return r;
+}
+
+static struct platform_driver clk_mt8195_drv = {
+       .probe = clk_mt8195_probe,
+       .driver = {
+               .name = "clk-mt8195",
+               .of_match_table = of_match_clk_mt8195,
+       },
+};
+
+static int __init clk_mt8195_init(void)
+{
+       return platform_driver_register(&clk_mt8195_drv);
+}
+
+arch_initcall(clk_mt8195_init);
Is there any particular reason for arch_initcall?

APMIXEDSYS/TOPCKGEN are clock source of others IP block and
PERICFG/INFRACFG provide peripheral and bus clocks control, so we
want
to init early.
Sure, but this should really be done through standard dependency
handling, and not trying to sequence them by hand.

Is there an observable benefit to having arch_initcall() vs the
standard
order with builtin_platform_driver()? If so, this should be
documented
here as a justifcation for arch_initcall().
I will use builtin_platform_driver like other subsys ip block
in next version.

Best Regards,
Chun-Jie

Regards
ChenYu
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help

Possibly related (same subject, not in this thread)