| Message ID | 23bc89d407e7797e97b703fa939b43bfe79296ce.1701823757.git.daniel@makrotopia.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v3,1/4] dt-bindings: clock: mediatek: add MT7988 clock IDs | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success |
On 06/12/2023 01:55, Daniel Golle wrote: > From: Sam Shih <sam.shih@mediatek.com> > > Add MT7988 clock dt-bindings for topckgen, apmixedsys, infracfg, > ethernet and xfipll subsystem clocks. > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > v3: no changes Really? It was entirely different author at v2. > v2: fix indentation > > .../dt-bindings/clock/mediatek,mt7988-clk.h | 280 ++++++++++++++++++ > 1 file changed, 280 insertions(+) > create mode 100644 include/dt-bindings/clock/mediatek,mt7988-clk.h > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Il 06/12/23 01:57, Daniel Golle ha scritto: > From: Sam Shih <sam.shih@mediatek.com> > > Introduce pcw_chg_shfit control to optionally use that instead of the > hardcoded PCW_CHG_MASK macro. > This will needed for clocks on the MT7988 SoC. > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > v3: use git --from ... > v2: no changes > > drivers/clk/mediatek/clk-pll.c | 5 ++++- > drivers/clk/mediatek/clk-pll.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c > index 513ab6b1b3229..9f08bc5d2a8a2 100644 > --- a/drivers/clk/mediatek/clk-pll.c > +++ b/drivers/clk/mediatek/clk-pll.c > @@ -114,7 +114,10 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw, > pll->data->pcw_shift); > val |= pcw << pll->data->pcw_shift; > writel(val, pll->pcw_addr); > - chg = readl(pll->pcw_chg_addr) | PCW_CHG_MASK; > + if (pll->data->pcw_chg_shift) > + chg = readl(pll->pcw_chg_addr) | BIT(pll->data->pcw_chg_shift); > + else > + chg = readl(pll->pcw_chg_addr) | PCW_CHG_MASK; > writel(chg, pll->pcw_chg_addr); > if (pll->tuner_addr) > writel(val + 1, pll->tuner_addr); > diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h > index f17278ff15d78..d28d317e84377 100644 > --- a/drivers/clk/mediatek/clk-pll.h > +++ b/drivers/clk/mediatek/clk-pll.h > @@ -44,6 +44,7 @@ struct mtk_pll_data { > u32 pcw_reg; > int pcw_shift; > u32 pcw_chg_reg; > + int pcw_chg_shift; > const struct mtk_pll_div_table *div_table; > const char *parent_name; > u32 en_reg; Hmm... no, this is not the best at all and can be improved. Okay, so, the situation here is that one or some PLL(s) on MT7988 have a different PCW_CHG_MASK as far as I understand. Situation here is: - Each PLL must be registered to clk-pll - Each driver declaring a PLL does exactly so - There's a function to register the PLL You definitely don't want to add a conditional in pll_set_rate(): even though this is technically not a performance path on the current SoCs (and will probably never be), it's simply useless to have this (very small) overhead there. The solution is to: - Change that pcw_chg_shift to an unsigned short int type (or u8, your call): you don't need 32 bits for this number, as the expected range of this member is [0-31], and this can be expressed in just 4 bits (u8 is the smallest though) - Add that to function mtk_clk_register_pll_ops() - Change mtk_pll_set_rate_regs() to always do chg = readl(pll->pcw_chg_addr) | BIT(pll->data->pcw_chg_shift); Cheers, Angelo
Il 06/12/23 01:57, Daniel Golle ha scritto: > From: Sam Shih <sam.shih@mediatek.com> > > Add APMIXED, ETH, INFRACFG and TOPCKGEN clock drivers which are > typical MediaTek designs. > > Also add driver for XFIPLL clock generating the 156.25MHz clock for > the XFI SerDes. It needs an undocumented software workaround and has > an unknown internal design. > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > v3: use git --from ... > v2: no changes > > drivers/clk/mediatek/Kconfig | 9 + > drivers/clk/mediatek/Makefile | 5 + > drivers/clk/mediatek/clk-mt7988-apmixed.c | 113 ++++++ > drivers/clk/mediatek/clk-mt7988-eth.c | 141 +++++++ > drivers/clk/mediatek/clk-mt7988-infracfg.c | 378 +++++++++++++++++ > drivers/clk/mediatek/clk-mt7988-topckgen.c | 446 +++++++++++++++++++++ > drivers/clk/mediatek/clk-mt7988-xfipll.c | 79 ++++ > 7 files changed, 1171 insertions(+) > create mode 100644 drivers/clk/mediatek/clk-mt7988-apmixed.c > create mode 100644 drivers/clk/mediatek/clk-mt7988-eth.c > create mode 100644 drivers/clk/mediatek/clk-mt7988-infracfg.c > create mode 100644 drivers/clk/mediatek/clk-mt7988-topckgen.c > create mode 100644 drivers/clk/mediatek/clk-mt7988-xfipll.c > > diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig > index 48b42d11111cd..70a005e7e1b18 100644 > --- a/drivers/clk/mediatek/Kconfig > +++ b/drivers/clk/mediatek/Kconfig > @@ -423,6 +423,15 @@ config COMMON_CLK_MT7986_ETHSYS > This driver adds support for clocks for Ethernet and SGMII > required on MediaTek MT7986 SoC. > > +config COMMON_CLK_MT7988 > + tristate "Clock driver for MediaTek MT7988" > + depends on ARCH_MEDIATEK || COMPILE_TEST > + select COMMON_CLK_MEDIATEK > + default ARCH_MEDIATEK > + help > + This driver supports MediaTek MT7988 basic clocks and clocks > + required for various periperals found on this SoC. > + > config COMMON_CLK_MT8135 > tristate "Clock driver for MediaTek MT8135" > depends on (ARCH_MEDIATEK && ARM) || COMPILE_TEST > diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile > index dbeaa5b41177d..eeccfa039896f 100644 > --- a/drivers/clk/mediatek/Makefile > +++ b/drivers/clk/mediatek/Makefile > @@ -62,6 +62,11 @@ obj-$(CONFIG_COMMON_CLK_MT7986) += clk-mt7986-apmixed.o > obj-$(CONFIG_COMMON_CLK_MT7986) += clk-mt7986-topckgen.o > obj-$(CONFIG_COMMON_CLK_MT7986) += clk-mt7986-infracfg.o > obj-$(CONFIG_COMMON_CLK_MT7986_ETHSYS) += clk-mt7986-eth.o > +obj-$(CONFIG_COMMON_CLK_MT7988) += clk-mt7988-apmixed.o > +obj-$(CONFIG_COMMON_CLK_MT7988) += clk-mt7988-topckgen.o > +obj-$(CONFIG_COMMON_CLK_MT7988) += clk-mt7988-infracfg.o > +obj-$(CONFIG_COMMON_CLK_MT7988) += clk-mt7988-eth.o > +obj-$(CONFIG_COMMON_CLK_MT7988) += clk-mt7988-xfipll.o > obj-$(CONFIG_COMMON_CLK_MT8135) += clk-mt8135-apmixedsys.o clk-mt8135.o > obj-$(CONFIG_COMMON_CLK_MT8167) += clk-mt8167-apmixedsys.o clk-mt8167.o > obj-$(CONFIG_COMMON_CLK_MT8167_AUDSYS) += clk-mt8167-aud.o > diff --git a/drivers/clk/mediatek/clk-mt7988-apmixed.c b/drivers/clk/mediatek/clk-mt7988-apmixed.c > new file mode 100644 > index 0000000000000..3f1edc231e37a > --- /dev/null > +++ b/drivers/clk/mediatek/clk-mt7988-apmixed.c > @@ -0,0 +1,113 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2023 MediaTek Inc. > + * Author: Sam Shih <sam.shih@mediatek.com> > + * Author: Xiufeng Li <Xiufeng.Li@mediatek.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 "clk-pll.h" > +#include <dt-bindings/clock/mediatek,mt7988-clk.h> > + > +#define MT7988_PLL_FMAX (2500UL * MHZ) > +#define MT7988_PCW_CHG_SHIFT 2 > + > +#define PLL_xtal(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _rst_bar_mask, \ You're never assigning any div_table, so you don't need to have PLL and PLL_xtal. Also, why is your crystal named "clkxtal" instead of "clk26m" like all the others? If your clkxtal is 26MHz, please just change the name to "clk26m" as this is what is used by most of the other MTK clock drivers, and because the clk26m name does actually stand for practically the same as your clkxtal, granted that yours is 26M. > + _pcwbits, _pd_reg, _pd_shift, _tuner_reg, _tuner_en_reg, \ > + _tuner_en_bit, _pcw_reg, _pcw_shift, _pcw_chg_reg, \ > + _div_table) \ > + { \ > + .id = _id, .name = _name, .reg = _reg, .pwr_reg = _pwr_reg, \ > + .en_mask = _en_mask, .flags = _flags, \ > + .rst_bar_mask = BIT(_rst_bar_mask), .fmax = MT7988_PLL_FMAX, \ > + .pcwbits = _pcwbits, .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, \ > + .pcw_chg_shift = MT7988_PCW_CHG_SHIFT, \ > + .div_table = _div_table, .parent_name = "clkxtal", \ > + } > + ..snip.. > + > +static const struct of_device_id of_match_clk_mt7988_apmixed[] = { > + { .compatible = "mediatek,mt7988-apmixedsys", }, { .compatible = "mediatek,mt7988-apmixedsys" }, ( there is an extra comma: `sys", }` => `sys" }` ) > + { /* sentinel */ } > +}; > + > +static int clk_mt7988_apmixed_probe(struct platform_device *pdev) > +{ > + struct clk_hw_onecell_data *clk_data; > + struct device_node *node = pdev->dev.of_node; > + int r; > + > + clk_data = mtk_alloc_clk_data(ARRAY_SIZE(plls)); > + if (!clk_data) > + return -ENOMEM; > + > + mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data); Error checking is missing for mtk_clk_register_plls(). > + > + r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data); > + if (r) { > + pr_err("%s(): could not register clock provider: %d\n", > + __func__, r); > + goto free_apmixed_data; > + } > + return r; > + > +free_apmixed_data: > + mtk_free_clk_data(clk_data); > + return r; > +} > + > +static struct platform_driver clk_mt7988_apmixed_drv = { > + .probe = clk_mt7988_apmixed_probe, > + .driver = { > + .name = "clk-mt7988-apmixed", > + .of_match_table = of_match_clk_mt7988_apmixed, > + }, > +}; > +builtin_platform_driver(clk_mt7988_apmixed_drv); > +MODULE_LICENSE("GPL"); ..snip.. > diff --git a/drivers/clk/mediatek/clk-mt7988-infracfg.c b/drivers/clk/mediatek/clk-mt7988-infracfg.c > new file mode 100644 > index 0000000000000..d23f7bdfafd9d > --- /dev/null > +++ b/drivers/clk/mediatek/clk-mt7988-infracfg.c > @@ -0,0 +1,378 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2023 MediaTek Inc. > + * Author: Sam Shih <sam.shih@mediatek.com> > + * Author: Xiufeng Li <Xiufeng.Li@mediatek.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/mediatek,mt7988-clk.h> > + > +static DEFINE_SPINLOCK(mt7988_clk_lock); > + > +static const char *const infra_mux_uart0_parents[] __initconst = { Are you sure that you need those __initconst annotations? > + "csw_infra_f26m_sel", "uart_sel" > +}; > + ..snip.. > + > +static const struct mtk_gate_regs infra0_cg_regs = { > + .set_ofs = 0x10, > + .clr_ofs = 0x14, > + .sta_ofs = 0x18, > +}; > + > +static const struct mtk_gate_regs infra1_cg_regs = { > + .set_ofs = 0x40, > + .clr_ofs = 0x44, > + .sta_ofs = 0x48, > +}; > + > +static const struct mtk_gate_regs infra2_cg_regs = { > + .set_ofs = 0x50, > + .clr_ofs = 0x54, > + .sta_ofs = 0x58, > +}; > + > +static const struct mtk_gate_regs infra3_cg_regs = { > + .set_ofs = 0x60, > + .clr_ofs = 0x64, > + .sta_ofs = 0x68, > +}; > + > +#define GATE_INFRA0_FLAGS(_id, _name, _parent, _shift, _flags) \ This is exactly the definition of GATE_MTK_FLAGS. Use that common definition instead. > + { \ > + .id = _id, .name = _name, .parent_name = _parent, \ > + .regs = &infra0_cg_regs, .shift = _shift, \ > + .ops = &mtk_clk_gate_ops_setclr, .flags = _flags, \ > + } > + > +#define GATE_INFRA1_FLAGS(_id, _name, _parent, _shift, _flags) \ same > + { \ > + .id = _id, .name = _name, .parent_name = _parent, \ > + .regs = &infra1_cg_regs, .shift = _shift, \ > + .ops = &mtk_clk_gate_ops_setclr, .flags = _flags, \ > + } > + > +#define GATE_INFRA2_FLAGS(_id, _name, _parent, _shift, _flags) \ same again > + { \ > + .id = _id, .name = _name, .parent_name = _parent, \ > + .regs = &infra2_cg_regs, .shift = _shift, \ > + .ops = &mtk_clk_gate_ops_setclr, .flags = _flags, \ > + } > + > +#define GATE_INFRA3_FLAGS(_id, _name, _parent, _shift, _flags) \ and again. > + { \ > + .id = _id, .name = _name, .parent_name = _parent, \ > + .regs = &infra3_cg_regs, .shift = _shift, \ > + .ops = &mtk_clk_gate_ops_setclr, .flags = _flags, \ > + } > + #define GATE_INFRA0(_id, _name, _parent, _shift) \ GATE_MTK_FLAGS(_id, _name, _parent, &infra0_cg_regs, _shift, \ &mtk_clk_gate_ops_setclr, 0) #define GATE_INFRA1(_id, _name, _parent, _shift) \ GATE_MTK_FLAGS(_id, _name, _parent, &infra1_cg_regs, _shift, \ &mtk_clk_gate_ops_setclr, 0) #define GATE_INFRA2_FLAGS(_id, _name, _parent, _shift, _flags) \ GATE_MTK_FLAGS(_id, _name, _parent, &infra2_cg_regs, _shift, \ &mtk_clk_gate_ops_setclr, _flags) #define GATE_INFRA2(_id, _name, _parent, _shift) \ GATE_INFRA2_FLAGS(_id, _name, _parent, _shift, 0) #define GATE_INFRA3(_id, _name, _parent, _shift) \ GATE_MTK_FLAGS(_id, _name, _parent, &infra0_cg_regs, _shift, \ &mtk_clk_gate_ops_setclr, 0) > +#define GATE_INFRA0(_id, _name, _parent, _shift) \ > + GATE_INFRA0_FLAGS(_id, _name, _parent, _shift, 0) > + > +#define GATE_INFRA1(_id, _name, _parent, _shift) \ > + GATE_INFRA1_FLAGS(_id, _name, _parent, _shift, 0) > + > +#define GATE_INFRA2(_id, _name, _parent, _shift) \ > + GATE_INFRA2_FLAGS(_id, _name, _parent, _shift, 0) > + > +#define GATE_INFRA3(_id, _name, _parent, _shift) \ > + GATE_INFRA3_FLAGS(_id, _name, _parent, _shift, 0) > + > + > +#define GATE_CRITICAL(_id, _name, _parent, _regs, _shift) \ You are using it like that: GATE_CRITICAL(CLK_INFRA_RTC, "infra_f_frtc", "top_rtc_32k", &infra2_cg_regs, 19), Change it to GATE_INFRA2_FLAGS(CLK_INFRA_RTC, "infra_f_frtc", "top_rtc_32k", 19), > + { \ > + .id = _id, .name = _name, .parent_name = _parent, \ > + .regs = _regs, .shift = _shift, \ > + .flags = CLK_IS_CRITICAL, \ > + .ops = &mtk_clk_gate_ops_setclr, \ > + } > + > +static const struct mtk_gate infra_clks[] = { > + /* INFRA0 */ > + GATE_INFRA0(CLK_INFRA_PCIE_PERI_26M_CK_P0, > + "infra_pcie_peri_ck_26m_ck_p0", "csw_infra_f26m_sel", 7), > + GATE_INFRA0(CLK_INFRA_PCIE_PERI_26M_CK_P1, > + "infra_pcie_peri_ck_26m_ck_p1", "csw_infra_f26m_sel", 8), > + GATE_INFRA0(CLK_INFRA_PCIE_PERI_26M_CK_P2, > + "infra_pcie_peri_ck_26m_ck_p2", "csw_infra_f26m_sel", 9), > + GATE_INFRA0(CLK_INFRA_PCIE_PERI_26M_CK_P3, > + "infra_pcie_peri_ck_26m_ck_p3", "csw_infra_f26m_sel", 10), > + /* INFRA1 */ > + GATE_INFRA1(CLK_INFRA_66M_GPT_BCK, "infra_hf_66m_gpt_bck", > + "sysaxi_sel", 0), This one and multiple other entries do fit in one line. Up to 100 columns is ok: please use one line where possible, here and in the other drivers as well, where this comment applies. > + GATE_INFRA1(CLK_INFRA_66M_PWM_HCK, "infra_hf_66m_pwm_hck", > + "sysaxi_sel", 1), ..snip.. > diff --git a/drivers/clk/mediatek/clk-mt7988-topckgen.c b/drivers/clk/mediatek/clk-mt7988-topckgen.c > new file mode 100644 > index 0000000000000..7bed282e615f8 > --- /dev/null > +++ b/drivers/clk/mediatek/clk-mt7988-topckgen.c > @@ -0,0 +1,446 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2023 MediaTek Inc. > + * Author: Sam Shih <sam.shih@mediatek.com> > + * Author: Xiufeng Li <Xiufeng.Li@mediatek.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/mediatek,mt7988-clk.h> > + > +static DEFINE_SPINLOCK(mt7988_clk_lock); > + > +static const struct mtk_fixed_clk top_fixed_clks[] = { > + FIXED_CLK(CLK_TOP_XTAL, "top_xtal", "clkxtal", 40000000), Again, why clkxtal and not clk26m? > +}; > + > +static const struct mtk_fixed_factor top_divs[] = { > + FACTOR(CLK_TOP_XTAL_D2, "top_xtal_d2", "top_xtal", 1, 2), > + FACTOR(CLK_TOP_RTC_32K, "top_rtc_32k", "top_xtal", 1, 1250), > + FACTOR(CLK_TOP_RTC_32P7K, "top_rtc_32p7k", "top_xtal", 1, 1220), > + FACTOR(CLK_TOP_MPLL_D2, "mpll_d2", "mpll", 1, 2), > + FACTOR(CLK_TOP_MPLL_D3_D2, "mpll_d3_d2", "mpll", 1, 2), > + FACTOR(CLK_TOP_MPLL_D4, "mpll_d4", "mpll", 1, 4), > + FACTOR(CLK_TOP_MPLL_D8, "mpll_d8", "mpll", 1, 8), > + FACTOR(CLK_TOP_MPLL_D8_D2, "mpll_d8_d2", "mpll", 1, 16), > + FACTOR(CLK_TOP_MMPLL_D2, "mmpll_d2", "mmpll", 1, 2), > + FACTOR(CLK_TOP_MMPLL_D3_D5, "mmpll_d3_d5", "mmpll", 1, 15), > + FACTOR(CLK_TOP_MMPLL_D4, "mmpll_d4", "mmpll", 1, 4), > + FACTOR(CLK_TOP_MMPLL_D6_D2, "mmpll_d6_d2", "mmpll", 1, 12), > + FACTOR(CLK_TOP_MMPLL_D8, "mmpll_d8", "mmpll", 1, 8), > + FACTOR(CLK_TOP_APLL2_D4, "apll2_d4", "apll2", 1, 4), > + FACTOR(CLK_TOP_NET1PLL_D4, "net1pll_d4", "net1pll", 1, 4), > + FACTOR(CLK_TOP_NET1PLL_D5, "net1pll_d5", "net1pll", 1, 5), > + FACTOR(CLK_TOP_NET1PLL_D5_D2, "net1pll_d5_d2", "net1pll", 1, 10), > + FACTOR(CLK_TOP_NET1PLL_D5_D4, "net1pll_d5_d4", "net1pll", 1, 20), > + FACTOR(CLK_TOP_NET1PLL_D8, "net1pll_d8", "net1pll", 1, 8), > + FACTOR(CLK_TOP_NET1PLL_D8_D2, "net1pll_d8_d2", "net1pll", 1, 16), > + FACTOR(CLK_TOP_NET1PLL_D8_D4, "net1pll_d8_d4", "net1pll", 1, 32), > + FACTOR(CLK_TOP_NET1PLL_D8_D8, "net1pll_d8_d8", "net1pll", 1, 64), > + FACTOR(CLK_TOP_NET1PLL_D8_D16, "net1pll_d8_d16", "net1pll", 1, 128), > + FACTOR(CLK_TOP_NET2PLL_D2, "net2pll_d2", "net2pll", 1, 2), > + FACTOR(CLK_TOP_NET2PLL_D4, "net2pll_d4", "net2pll", 1, 4), > + FACTOR(CLK_TOP_NET2PLL_D4_D4, "net2pll_d4_d4", "net2pll", 1, 16), > + FACTOR(CLK_TOP_NET2PLL_D4_D8, "net2pll_d4_d8", "net2pll", 1, 32), > + FACTOR(CLK_TOP_NET2PLL_D6, "net2pll_d6", "net2pll", 1, 6), > + FACTOR(CLK_TOP_NET2PLL_D8, "net2pll_d8", "net2pll", 1, 8), > +}; > + > +static const char *const netsys_parents[] = { "top_xtal", "net2pll_d2", > + "mmpll_d2" }; Fits in one line as well: please fix, here and everywhere else. > + > +static const char *const netsys_500m_parents[] = { "top_xtal", "net1pll_d5", > + "net1pll_d5_d2" }; > + ..snip.. > diff --git a/drivers/clk/mediatek/clk-mt7988-xfipll.c b/drivers/clk/mediatek/clk-mt7988-xfipll.c > new file mode 100644 > index 0000000000000..5862818085a8c > --- /dev/null > +++ b/drivers/clk/mediatek/clk-mt7988-xfipll.c > @@ -0,0 +1,79 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2023 Daniel Golle <daniel@makrotopia.org> > + */ > + > +#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 <dt-bindings/clock/mediatek,mt7988-clk.h> > + > +/* Register to control USXGMII XFI PLL analog */ > +#define XFI_PLL_ANA_GLB8 0x108 > +#define RG_XFI_PLL_ANA_SWWA 0x02283248 > + > +static const struct mtk_gate_regs xfipll_cg_regs = { > + .set_ofs = 0x8, > + .clr_ofs = 0x8, > + .sta_ofs = 0x8, > +}; > + > +#define GATE_XFIPLL(_id, _name, _parent, _shift) \ > + { \ > + .id = _id, .name = _name, .parent_name = _parent, \ > + .regs = &xfipll_cg_regs, .shift = _shift, \ > + .ops = &mtk_clk_gate_ops_no_setclr_inv, \ > + } > + > +static const struct mtk_fixed_factor xfipll_divs[] = { > + FACTOR(CLK_XFIPLL_PLL, "xfipll_pll", "top_xtal", 125, 32), > +}; > + > +static const struct mtk_gate xfipll_clks[] = { > + GATE_XFIPLL(CLK_XFIPLL_PLL_EN, "xfipll_pll_en", "xfipll_pll", 31), > +}; > + > +static const struct mtk_clk_desc xfipll_desc = { > + .clks = xfipll_clks, > + .num_clks = ARRAY_SIZE(xfipll_clks), > + .factor_clks = xfipll_divs, > + .num_factor_clks = ARRAY_SIZE(xfipll_divs), > +}; > + > +static int clk_mt7988_xfipll_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node; > + void __iomem *base = of_iomap(node, 0); > + > + if (!base) > + return -ENOMEM; > + > + /* Apply software workaround for USXGMII PLL TCL issue */ù This hurts my heart... :-P but it's ok :-) Cheers, Angelo
Hi Angelo, thank you for taking the time to review and for the helpful comments. On Wed, Dec 06, 2023 at 11:38:36AM +0100, AngeloGioacchino Del Regno wrote: > Il 06/12/23 01:57, Daniel Golle ha scritto: > > From: Sam Shih <sam.shih@mediatek.com> > > > > Introduce pcw_chg_shfit control to optionally use that instead of the > > hardcoded PCW_CHG_MASK macro. > > This will needed for clocks on the MT7988 SoC. > > > > Signed-off-by: Sam Shih <sam.shih@mediatek.com> > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > --- > > v3: use git --from ... > > v2: no changes > > > > drivers/clk/mediatek/clk-pll.c | 5 ++++- > > drivers/clk/mediatek/clk-pll.h | 1 + > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c > > index 513ab6b1b3229..9f08bc5d2a8a2 100644 > > --- a/drivers/clk/mediatek/clk-pll.c > > +++ b/drivers/clk/mediatek/clk-pll.c > > @@ -114,7 +114,10 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw, > > pll->data->pcw_shift); > > val |= pcw << pll->data->pcw_shift; > > writel(val, pll->pcw_addr); > > - chg = readl(pll->pcw_chg_addr) | PCW_CHG_MASK; > > + if (pll->data->pcw_chg_shift) > > + chg = readl(pll->pcw_chg_addr) | BIT(pll->data->pcw_chg_shift); > > + else > > + chg = readl(pll->pcw_chg_addr) | PCW_CHG_MASK; > > writel(chg, pll->pcw_chg_addr); > > if (pll->tuner_addr) > > writel(val + 1, pll->tuner_addr); > > diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h > > index f17278ff15d78..d28d317e84377 100644 > > --- a/drivers/clk/mediatek/clk-pll.h > > +++ b/drivers/clk/mediatek/clk-pll.h > > @@ -44,6 +44,7 @@ struct mtk_pll_data { > > u32 pcw_reg; > > int pcw_shift; > > u32 pcw_chg_reg; > > + int pcw_chg_shift; > > const struct mtk_pll_div_table *div_table; > > const char *parent_name; > > u32 en_reg; > > Hmm... no, this is not the best at all and can be improved. > > Okay, so, the situation here is that one or some PLL(s) on MT7988 have a different > PCW_CHG_MASK as far as I understand. Correct. *All* clocks of MT7988 have a different PCW_CHG_MASK, BIT(2) instead of BIT(31). > > Situation here is: > - Each PLL must be registered to clk-pll > - Each driver declaring a PLL does exactly so > - There's a function to register the PLL > > You definitely don't want to add a conditional in pll_set_rate(): even though > this is technically not a performance path on the current SoCs (and will probably > never be), it's simply useless to have this (very small) overhead there. > > The solution is to: > - Change that pcw_chg_shift to an unsigned short int type (or u8, your call): > you don't need 32 bits for this number, as the expected range of this member > is [0-31], and this can be expressed in just 4 bits (u8 is the smallest though) Ack will use u8 instead, despite the struct not being packed, so I wonder if it actually makes a difference. > - Add that to function mtk_clk_register_pll_ops() > - Change mtk_pll_set_rate_regs() to always do > chg = readl(pll->pcw_chg_addr) | BIT(pll->data->pcw_chg_shift); As mtk_pll_data is a read-only member of the mtk_pll struct, we can't set pcw_chg_shift to 31 in mtk_clk_register_pll_ops() in case it is set to 0. The only (much more intrusive change) would be to explicitely declare .pcw_chg_shift = 31 in all current drivers setting .pcs_chg_reg != 0. Should I do that instead?
diff --git a/include/dt-bindings/clock/mediatek,mt7988-clk.h b/include/dt-bindings/clock/mediatek,mt7988-clk.h new file mode 100644 index 0000000000000..63376e40f14d2 --- /dev/null +++ b/include/dt-bindings/clock/mediatek,mt7988-clk.h @@ -0,0 +1,280 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ +/* + * Copyright (c) 2023 MediaTek Inc. + * Author: Sam Shih <sam.shih@mediatek.com> + * Author: Xiufeng Li <Xiufeng.Li@mediatek.com> + */ + +#ifndef _DT_BINDINGS_CLK_MT7988_H +#define _DT_BINDINGS_CLK_MT7988_H + +/* APMIXEDSYS */ + +#define CLK_APMIXED_NETSYSPLL 0 +#define CLK_APMIXED_MPLL 1 +#define CLK_APMIXED_MMPLL 2 +#define CLK_APMIXED_APLL2 3 +#define CLK_APMIXED_NET1PLL 4 +#define CLK_APMIXED_NET2PLL 5 +#define CLK_APMIXED_WEDMCUPLL 6 +#define CLK_APMIXED_SGMPLL 7 +#define CLK_APMIXED_ARM_B 8 +#define CLK_APMIXED_CCIPLL2_B 9 +#define CLK_APMIXED_USXGMIIPLL 10 +#define CLK_APMIXED_MSDCPLL 11 + +/* TOPCKGEN */ + +#define CLK_TOP_XTAL 0 +#define CLK_TOP_XTAL_D2 1 +#define CLK_TOP_RTC_32K 2 +#define CLK_TOP_RTC_32P7K 3 +#define CLK_TOP_MPLL_D2 4 +#define CLK_TOP_MPLL_D3_D2 5 +#define CLK_TOP_MPLL_D4 6 +#define CLK_TOP_MPLL_D8 7 +#define CLK_TOP_MPLL_D8_D2 8 +#define CLK_TOP_MMPLL_D2 9 +#define CLK_TOP_MMPLL_D3_D5 10 +#define CLK_TOP_MMPLL_D4 11 +#define CLK_TOP_MMPLL_D6_D2 12 +#define CLK_TOP_MMPLL_D8 13 +#define CLK_TOP_APLL2_D4 14 +#define CLK_TOP_NET1PLL_D4 15 +#define CLK_TOP_NET1PLL_D5 16 +#define CLK_TOP_NET1PLL_D5_D2 17 +#define CLK_TOP_NET1PLL_D5_D4 18 +#define CLK_TOP_NET1PLL_D8 19 +#define CLK_TOP_NET1PLL_D8_D2 20 +#define CLK_TOP_NET1PLL_D8_D4 21 +#define CLK_TOP_NET1PLL_D8_D8 22 +#define CLK_TOP_NET1PLL_D8_D16 23 +#define CLK_TOP_NET2PLL_D2 24 +#define CLK_TOP_NET2PLL_D4 25 +#define CLK_TOP_NET2PLL_D4_D4 26 +#define CLK_TOP_NET2PLL_D4_D8 27 +#define CLK_TOP_NET2PLL_D6 28 +#define CLK_TOP_NET2PLL_D8 29 +#define CLK_TOP_NETSYS_SEL 30 +#define CLK_TOP_NETSYS_500M_SEL 31 +#define CLK_TOP_NETSYS_2X_SEL 32 +#define CLK_TOP_NETSYS_GSW_SEL 33 +#define CLK_TOP_ETH_GMII_SEL 34 +#define CLK_TOP_NETSYS_MCU_SEL 35 +#define CLK_TOP_NETSYS_PAO_2X_SEL 36 +#define CLK_TOP_EIP197_SEL 37 +#define CLK_TOP_AXI_INFRA_SEL 38 +#define CLK_TOP_UART_SEL 39 +#define CLK_TOP_EMMC_250M_SEL 40 +#define CLK_TOP_EMMC_400M_SEL 41 +#define CLK_TOP_SPI_SEL 42 +#define CLK_TOP_SPIM_MST_SEL 43 +#define CLK_TOP_NFI1X_SEL 44 +#define CLK_TOP_SPINFI_SEL 45 +#define CLK_TOP_PWM_SEL 46 +#define CLK_TOP_I2C_SEL 47 +#define CLK_TOP_PCIE_MBIST_250M_SEL 48 +#define CLK_TOP_PEXTP_TL_SEL 49 +#define CLK_TOP_PEXTP_TL_P1_SEL 50 +#define CLK_TOP_PEXTP_TL_P2_SEL 51 +#define CLK_TOP_PEXTP_TL_P3_SEL 52 +#define CLK_TOP_USB_SYS_SEL 53 +#define CLK_TOP_USB_SYS_P1_SEL 54 +#define CLK_TOP_USB_XHCI_SEL 55 +#define CLK_TOP_USB_XHCI_P1_SEL 56 +#define CLK_TOP_USB_FRMCNT_SEL 57 +#define CLK_TOP_USB_FRMCNT_P1_SEL 58 +#define CLK_TOP_AUD_SEL 59 +#define CLK_TOP_A1SYS_SEL 60 +#define CLK_TOP_AUD_L_SEL 61 +#define CLK_TOP_A_TUNER_SEL 62 +#define CLK_TOP_SSPXTP_SEL 63 +#define CLK_TOP_USB_PHY_SEL 64 +#define CLK_TOP_USXGMII_SBUS_0_SEL 65 +#define CLK_TOP_USXGMII_SBUS_1_SEL 66 +#define CLK_TOP_SGM_0_SEL 67 +#define CLK_TOP_SGM_SBUS_0_SEL 68 +#define CLK_TOP_SGM_1_SEL 69 +#define CLK_TOP_SGM_SBUS_1_SEL 70 +#define CLK_TOP_XFI_PHY_0_XTAL_SEL 71 +#define CLK_TOP_XFI_PHY_1_XTAL_SEL 72 +#define CLK_TOP_SYSAXI_SEL 73 +#define CLK_TOP_SYSAPB_SEL 74 +#define CLK_TOP_ETH_REFCK_50M_SEL 75 +#define CLK_TOP_ETH_SYS_200M_SEL 76 +#define CLK_TOP_ETH_SYS_SEL 77 +#define CLK_TOP_ETH_XGMII_SEL 78 +#define CLK_TOP_BUS_TOPS_SEL 79 +#define CLK_TOP_NPU_TOPS_SEL 80 +#define CLK_TOP_DRAMC_SEL 81 +#define CLK_TOP_DRAMC_MD32_SEL 82 +#define CLK_TOP_INFRA_F26M_SEL 83 +#define CLK_TOP_PEXTP_P0_SEL 84 +#define CLK_TOP_PEXTP_P1_SEL 85 +#define CLK_TOP_PEXTP_P2_SEL 86 +#define CLK_TOP_PEXTP_P3_SEL 87 +#define CLK_TOP_DA_XTP_GLB_P0_SEL 88 +#define CLK_TOP_DA_XTP_GLB_P1_SEL 89 +#define CLK_TOP_DA_XTP_GLB_P2_SEL 90 +#define CLK_TOP_DA_XTP_GLB_P3_SEL 91 +#define CLK_TOP_CKM_SEL 92 +#define CLK_TOP_DA_SEL 93 +#define CLK_TOP_PEXTP_SEL 94 +#define CLK_TOP_TOPS_P2_26M_SEL 95 +#define CLK_TOP_MCUSYS_BACKUP_625M_SEL 96 +#define CLK_TOP_NETSYS_SYNC_250M_SEL 97 +#define CLK_TOP_MACSEC_SEL 98 +#define CLK_TOP_NETSYS_TOPS_400M_SEL 99 +#define CLK_TOP_NETSYS_PPEFB_250M_SEL 100 +#define CLK_TOP_NETSYS_WARP_SEL 101 +#define CLK_TOP_ETH_MII_SEL 102 +#define CLK_TOP_NPU_SEL 103 +#define CLK_TOP_AUD_I2S_M 104 + +/* MCUSYS */ + +#define CLK_MCU_BUS_DIV_SEL 0 +#define CLK_MCU_ARM_DIV_SEL 1 + +/* INFRACFG_AO */ + +#define CLK_INFRA_MUX_UART0_SEL 0 +#define CLK_INFRA_MUX_UART1_SEL 1 +#define CLK_INFRA_MUX_UART2_SEL 2 +#define CLK_INFRA_MUX_SPI0_SEL 3 +#define CLK_INFRA_MUX_SPI1_SEL 4 +#define CLK_INFRA_MUX_SPI2_SEL 5 +#define CLK_INFRA_PWM_SEL 6 +#define CLK_INFRA_PWM_CK1_SEL 7 +#define CLK_INFRA_PWM_CK2_SEL 8 +#define CLK_INFRA_PWM_CK3_SEL 9 +#define CLK_INFRA_PWM_CK4_SEL 10 +#define CLK_INFRA_PWM_CK5_SEL 11 +#define CLK_INFRA_PWM_CK6_SEL 12 +#define CLK_INFRA_PWM_CK7_SEL 13 +#define CLK_INFRA_PWM_CK8_SEL 14 +#define CLK_INFRA_PCIE_GFMUX_TL_O_P0_SEL 15 +#define CLK_INFRA_PCIE_GFMUX_TL_O_P1_SEL 16 +#define CLK_INFRA_PCIE_GFMUX_TL_O_P2_SEL 17 +#define CLK_INFRA_PCIE_GFMUX_TL_O_P3_SEL 18 + +/* INFRACFG */ + +#define CLK_INFRA_PCIE_PERI_26M_CK_P0 19 +#define CLK_INFRA_PCIE_PERI_26M_CK_P1 20 +#define CLK_INFRA_PCIE_PERI_26M_CK_P2 21 +#define CLK_INFRA_PCIE_PERI_26M_CK_P3 22 +#define CLK_INFRA_66M_GPT_BCK 23 +#define CLK_INFRA_66M_PWM_HCK 24 +#define CLK_INFRA_66M_PWM_BCK 25 +#define CLK_INFRA_66M_PWM_CK1 26 +#define CLK_INFRA_66M_PWM_CK2 27 +#define CLK_INFRA_66M_PWM_CK3 28 +#define CLK_INFRA_66M_PWM_CK4 29 +#define CLK_INFRA_66M_PWM_CK5 30 +#define CLK_INFRA_66M_PWM_CK6 31 +#define CLK_INFRA_66M_PWM_CK7 32 +#define CLK_INFRA_66M_PWM_CK8 33 +#define CLK_INFRA_133M_CQDMA_BCK 34 +#define CLK_INFRA_66M_AUD_SLV_BCK 35 +#define CLK_INFRA_AUD_26M 36 +#define CLK_INFRA_AUD_L 37 +#define CLK_INFRA_AUD_AUD 38 +#define CLK_INFRA_AUD_EG2 39 +#define CLK_INFRA_DRAMC_F26M 40 +#define CLK_INFRA_133M_DBG_ACKM 41 +#define CLK_INFRA_66M_AP_DMA_BCK 42 +#define CLK_INFRA_66M_SEJ_BCK 43 +#define CLK_INFRA_PRE_CK_SEJ_F13M 44 +#define CLK_INFRA_26M_THERM_SYSTEM 45 +#define CLK_INFRA_I2C_BCK 46 +#define CLK_INFRA_52M_UART0_CK 47 +#define CLK_INFRA_52M_UART1_CK 48 +#define CLK_INFRA_52M_UART2_CK 49 +#define CLK_INFRA_NFI 50 +#define CLK_INFRA_SPINFI 51 +#define CLK_INFRA_66M_NFI_HCK 52 +#define CLK_INFRA_104M_SPI0 53 +#define CLK_INFRA_104M_SPI1 54 +#define CLK_INFRA_104M_SPI2_BCK 55 +#define CLK_INFRA_66M_SPI0_HCK 56 +#define CLK_INFRA_66M_SPI1_HCK 57 +#define CLK_INFRA_66M_SPI2_HCK 58 +#define CLK_INFRA_66M_FLASHIF_AXI 59 +#define CLK_INFRA_RTC 60 +#define CLK_INFRA_26M_ADC_BCK 61 +#define CLK_INFRA_RC_ADC 62 +#define CLK_INFRA_MSDC400 63 +#define CLK_INFRA_MSDC2_HCK 64 +#define CLK_INFRA_133M_MSDC_0_HCK 65 +#define CLK_INFRA_66M_MSDC_0_HCK 66 +#define CLK_INFRA_133M_CPUM_BCK 67 +#define CLK_INFRA_BIST2FPC 68 +#define CLK_INFRA_I2C_X16W_MCK_CK_P1 69 +#define CLK_INFRA_I2C_X16W_PCK_CK_P1 70 +#define CLK_INFRA_133M_USB_HCK 71 +#define CLK_INFRA_133M_USB_HCK_CK_P1 72 +#define CLK_INFRA_66M_USB_HCK 73 +#define CLK_INFRA_66M_USB_HCK_CK_P1 74 +#define CLK_INFRA_USB_SYS 75 +#define CLK_INFRA_USB_SYS_CK_P1 76 +#define CLK_INFRA_USB_REF 77 +#define CLK_INFRA_USB_CK_P1 78 +#define CLK_INFRA_USB_FRMCNT 79 +#define CLK_INFRA_USB_FRMCNT_CK_P1 80 +#define CLK_INFRA_USB_PIPE 81 +#define CLK_INFRA_USB_PIPE_CK_P1 82 +#define CLK_INFRA_USB_UTMI 83 +#define CLK_INFRA_USB_UTMI_CK_P1 84 +#define CLK_INFRA_USB_XHCI 85 +#define CLK_INFRA_USB_XHCI_CK_P1 86 +#define CLK_INFRA_PCIE_GFMUX_TL_P0 87 +#define CLK_INFRA_PCIE_GFMUX_TL_P1 88 +#define CLK_INFRA_PCIE_GFMUX_TL_P2 89 +#define CLK_INFRA_PCIE_GFMUX_TL_P3 90 +#define CLK_INFRA_PCIE_PIPE_P0 91 +#define CLK_INFRA_PCIE_PIPE_P1 92 +#define CLK_INFRA_PCIE_PIPE_P2 93 +#define CLK_INFRA_PCIE_PIPE_P3 94 +#define CLK_INFRA_133M_PCIE_CK_P0 95 +#define CLK_INFRA_133M_PCIE_CK_P1 96 +#define CLK_INFRA_133M_PCIE_CK_P2 97 +#define CLK_INFRA_133M_PCIE_CK_P3 98 + +/* ETHDMA */ + +#define CLK_ETHDMA_XGP1_EN 0 +#define CLK_ETHDMA_XGP2_EN 1 +#define CLK_ETHDMA_XGP3_EN 2 +#define CLK_ETHDMA_FE_EN 3 +#define CLK_ETHDMA_GP2_EN 4 +#define CLK_ETHDMA_GP1_EN 5 +#define CLK_ETHDMA_GP3_EN 6 +#define CLK_ETHDMA_ESW_EN 7 +#define CLK_ETHDMA_CRYPT0_EN 8 +#define CLK_ETHDMA_NR_CLK 9 + +/* SGMIISYS_0 */ + +#define CLK_SGM0_TX_EN 0 +#define CLK_SGM0_RX_EN 1 +#define CLK_SGMII0_NR_CLK 2 + +/* SGMIISYS_1 */ + +#define CLK_SGM1_TX_EN 0 +#define CLK_SGM1_RX_EN 1 +#define CLK_SGMII1_NR_CLK 2 + +/* ETHWARP */ + +#define CLK_ETHWARP_WOCPU2_EN 0 +#define CLK_ETHWARP_WOCPU1_EN 1 +#define CLK_ETHWARP_WOCPU0_EN 2 +#define CLK_ETHWARP_NR_CLK 3 + +/* XFIPLL */ +#define CLK_XFIPLL_PLL 0 +#define CLK_XFIPLL_PLL_EN 1 + +#endif /* _DT_BINDINGS_CLK_MT7988_H */