Re: [PATCH] clk: mediatek: add mt7981 clock support

[Daniel Golle <daniel@makrotopia.org>]

Hi Jianhui,

On Tue, Nov 15, 2022 at 02:06:06AM +0000, Jianhui Zhao wrote:
> Signed-off-by: Jianhui Zhao <zhaojh329@gmail.com>
> ---
>  .../bindings/clock/mediatek,apmixedsys.yaml   |   1 +
>  .../bindings/clock/mediatek,topckgen.yaml     |   1 +
>  drivers/clk/mediatek/Kconfig                  |  17 +
>  drivers/clk/mediatek/Makefile                 |   4 +
>  drivers/clk/mediatek/clk-mt7981-apmixed.c     | 103 +++++
>  drivers/clk/mediatek/clk-mt7981-eth.c         | 138 ++++++
>  drivers/clk/mediatek/clk-mt7981-infracfg.c    | 323 +++++++++++++
>  drivers/clk/mediatek/clk-mt7981-topckgen.c    | 431 ++++++++++++++++++
>  include/dt-bindings/clock/mt7981-clk.h        | 253 ++++++++++
>  9 files changed, 1271 insertions(+)
>  create mode 100644 drivers/clk/mediatek/clk-mt7981-apmixed.c
>  create mode 100644 drivers/clk/mediatek/clk-mt7981-eth.c
>  create mode 100644 drivers/clk/mediatek/clk-mt7981-infracfg.c
>  create mode 100644 drivers/clk/mediatek/clk-mt7981-topckgen.c
>  create mode 100644 include/dt-bindings/clock/mt7981-clk.h
> 
> ...
> diff --git a/drivers/clk/mediatek/clk-mt7981-infracfg.c b/drivers/clk/mediatek/clk-mt7981-infracfg.c
> new file mode 100644
> index 000000000000..d483e654606f
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt7981-infracfg.c
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021 MediaTek Inc.
> + * Author: Sam Shih <sam.shih@mediatek.com>
> + * Author: Wenzhen Yu <wenzhen.yu@mediatek.com>
> + * Author: Jianhui Zhao <zhaojh329@gmail.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include "clk-mtk.h"
> +#include "clk-gate.h"
> +#include "clk-mux.h"
> +
> +#include <dt-bindings/clock/mt7981-clk.h>
> +#include <linux/clk.h>
> +
> +static DEFINE_SPINLOCK(mt7981_clk_lock);
> +
> +static const struct mtk_fixed_factor infra_divs[] = {
> +	FACTOR(CLK_INFRA_CK_F26M, "infra_ck_f26m", "csw_f26m_sel", 1, 1),
> +	FACTOR(CLK_INFRA_UART, "infra_uart", "uart_sel", 1, 1),
> +	FACTOR(CLK_INFRA_ISPI0, "infra_ispi0", "spi_sel", 1, 1),
> +	FACTOR(CLK_INFRA_I2C, "infra_i2c", "i2c_sel", 1, 1),
> +	FACTOR(CLK_INFRA_ISPI1, "infra_ispi1", "spim_mst_sel", 1, 1),
> +	FACTOR(CLK_INFRA_PWM, "infra_pwm", "pwm_sel", 1, 1),
> +	FACTOR(CLK_INFRA_66M_MCK, "infra_66m_mck", "sysaxi_sel", 1, 2),
> +	FACTOR(CLK_INFRA_CK_F32K, "infra_ck_f32k", "cb_rtc_32p7k", 1, 1),
> +	FACTOR(CLK_INFRA_PCIE_CK, "infra_pcie", "pextp_tl_ck_sel", 1, 1),
> +	FACTOR(CLK_INFRA_PWM_BCK, "infra_pwm_bck", "infra_pwm_bsel", 1, 1),
> +	FACTOR(CLK_INFRA_PWM_CK1, "infra_pwm_ck1", "infra_pwm1_sel", 1, 1),
> +	FACTOR(CLK_INFRA_PWM_CK2, "infra_pwm_ck2", "infra_pwm2_sel", 1, 1),
> +	FACTOR(CLK_INFRA_133M_HCK, "infra_133m_hck", "sysaxi", 1, 1),
> +	FACTOR(CLK_INFRA_66M_PHCK, "infra_66m_phck", "infra_133m_hck", 1, 1),
> +	FACTOR(CLK_INFRA_FAUD_L_CK, "infra_faud_l", "aud_l", 1, 1),
> +	FACTOR(CLK_INFRA_FAUD_AUD_CK, "infra_faud_aud", "a1sys", 1, 1),
> +	FACTOR(CLK_INFRA_FAUD_EG2_CK, "infra_faud_eg2", "a_tuner", 1, 1),
> +	FACTOR(CLK_INFRA_I2CS_CK, "infra_i2cs", "i2c_bck", 1, 1),
> +	FACTOR(CLK_INFRA_MUX_UART0, "infra_mux_uart0", "infra_uart0_sel", 1, 1),
> +	FACTOR(CLK_INFRA_MUX_UART1, "infra_mux_uart1", "infra_uart1_sel", 1, 1),
> +	FACTOR(CLK_INFRA_MUX_UART2, "infra_mux_uart2", "infra_uart2_sel", 1, 1),
> +	FACTOR(CLK_INFRA_NFI_CK, "infra_nfi", "nfi1x", 1, 1),
> +	FACTOR(CLK_INFRA_SPINFI_CK, "infra_spinfi", "spinfi_bck", 1, 1),
> +	FACTOR(CLK_INFRA_MUX_SPI0, "infra_mux_spi0", "infra_spi0_sel", 1, 1),
> +	FACTOR(CLK_INFRA_MUX_SPI1, "infra_mux_spi1", "infra_spi1_sel", 1, 1),
> +	FACTOR(CLK_INFRA_MUX_SPI2, "infra_mux_spi2", "infra_spi2_sel", 1, 1),
> +	FACTOR(CLK_INFRA_RTC_32K, "infra_rtc_32k", "cb_rtc_32k", 1, 1),
> +	FACTOR(CLK_INFRA_FMSDC_CK, "infra_fmsdc", "emmc_400m", 1, 1),
> +	FACTOR(CLK_INFRA_FMSDC_HCK_CK, "infra_fmsdc_hck", "emmc_208m", 1, 1),
> +	FACTOR(CLK_INFRA_PERI_133M, "infra_peri_133m", "sysaxi", 1, 1),
> +	FACTOR(CLK_INFRA_133M_PHCK, "infra_133m_phck", "sysaxi", 1, 1),
> +	FACTOR(CLK_INFRA_USB_SYS_CK, "infra_usb_sys", "u2u3_sys", 1, 1),
> +	FACTOR(CLK_INFRA_USB_CK, "infra_usb", "u2u3_ref", 1, 1),
> +	FACTOR(CLK_INFRA_USB_XHCI_CK, "infra_usb_xhci", "u2u3_xhci", 1, 1),
> +	FACTOR(CLK_INFRA_PCIE_GFMUX_TL_O_PRE, "infra_pcie_mux", "pextp_tl", 1, 1),
> +	FACTOR(CLK_INFRA_F26M_CK0, "infra_f26m_ck0", "csw_f26m", 1, 1),
> +	FACTOR(CLK_INFRA_133M_MCK, "infra_133m_mck", "sysaxi", 1, 1),
> +};
> +

Just splitting the SDK driver into clock domains will have us end up
with lots of unnessesary aliases.
Please take a look at how clocks have been reorganized for MT7986:
Remove all duplicate 1:1 clock factors while keeping clock drivers
still working. Ie. there is no need to have both, infracfg and
infracfg_ao, for example, things can be simplified.

To understand what I mean in practise, please compare the clk-mt7986.c
driver from MediaTek SDK with the clk-mt7986-*.c drivers which have
been merged to Linux.

And did you actually test this? Can you publish your working tree for
MT7981 based on linux-next somewhere?
I'm asking because when I tried to do the same as you are doing now,
the MT7981 sample board I've been provided hangs early during boot...


Cheers


Daniel