* [PATCH V5 0/4] Add S4 SoC PLL and Peripheral clock controller
@ 2022-11-23 2:13 Yu Tu
2022-11-23 2:13 ` [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings Yu Tu
` (3 more replies)
0 siblings, 4 replies; 34+ messages in thread
From: Yu Tu @ 2022-11-23 2:13 UTC (permalink / raw)
To: linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel,
devicetree, Rob Herring, Neil Armstrong, Jerome Brunet,
Kevin Hilman, Michael Turquette, Stephen Boyd,
Krzysztof Kozlowski, Martin Blumenstingl
Cc: kelvin.zhang, Yu Tu
1. Add PLL and Peripheral clock controller driver for S4 SOC.
Yu Tu (4):
clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and
bindings
arm64: dts: meson: add S4 Soc PLL clock controller in DT
clk: meson: s4: add s4 SoC peripheral clock controller driver and
bindings
arm64: dts: meson: add S4 Soc Peripheral clock controller in DT
V4 -> V5: change format and clock flags and adjust the patch series as suggested
by Jerome.
V3 -> V4: change format and clock flags.
V2 -> V3: Use two clock controller.
V1 -> V2: Change format as discussed in the email.
Link:https://lore.kernel.org/all/20220823022630.25007-1-yu.tu@amlogic.com/
.../clock/amlogic,s4-peripherals-clkc.yaml | 105 +
.../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 +
MAINTAINERS | 1 +
arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 34 +
drivers/clk/meson/Kconfig | 25 +
drivers/clk/meson/Makefile | 2 +
drivers/clk/meson/s4-peripherals.c | 3783 +++++++++++++++++
drivers/clk/meson/s4-peripherals.h | 218 +
drivers/clk/meson/s4-pll.c | 875 ++++
drivers/clk/meson/s4-pll.h | 88 +
.../clock/amlogic,s4-peripherals-clkc.h | 131 +
.../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 +
12 files changed, 5343 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml
create mode 100644 drivers/clk/meson/s4-peripherals.c
create mode 100644 drivers/clk/meson/s4-peripherals.h
create mode 100644 drivers/clk/meson/s4-pll.c
create mode 100644 drivers/clk/meson/s4-pll.h
create mode 100644 include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h
create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h
base-commit: 5eeec1fd8360d57899d29a607ff81d0094e6cf59
--
2.33.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-23 2:13 [PATCH V5 0/4] Add S4 SoC PLL and Peripheral clock controller Yu Tu @ 2022-11-23 2:13 ` Yu Tu 2022-11-23 10:08 ` Krzysztof Kozlowski 2022-11-23 2:13 ` [PATCH V5 2/4] arm64: dts: meson: add S4 Soc PLL clock controller in DT Yu Tu ` (2 subsequent siblings) 3 siblings, 1 reply; 34+ messages in thread From: Yu Tu @ 2022-11-23 2:13 UTC (permalink / raw) To: linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang, Yu Tu Add the S4 PLL clock controller found and bindings in the s4 SoC family. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + MAINTAINERS | 1 + drivers/clk/meson/Kconfig | 13 + drivers/clk/meson/Makefile | 1 + drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ drivers/clk/meson/s4-pll.h | 88 ++ .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + 7 files changed, 1059 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml create mode 100644 drivers/clk/meson/s4-pll.c create mode 100644 drivers/clk/meson/s4-pll.h create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml new file mode 100644 index 000000000000..fd517e8ef14f --- /dev/null +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml @@ -0,0 +1,51 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Amlogic Meson S serials PLL Clock Controller + +maintainers: + - Neil Armstrong <narmstrong@baylibre.com> + - Jerome Brunet <jbrunet@baylibre.com> + - Yu Tu <yu.hu@amlogic.com> + + +properties: + compatible: + const: amlogic,s4-pll-clkc + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + items: + - const: xtal + + "#clock-cells": + const: 1 + +required: + - compatible + - reg + - clocks + - clock-names + - "#clock-cells" + +additionalProperties: false + +examples: + - | + clkc_pll: clock-controller@fe008000 { + compatible = "amlogic,s4-pll-clkc"; + reg = <0xfe008000 0x1e8>; + clocks = <&xtal>; + clock-names = "xtal"; + #clock-cells = <1>; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index cf0f18502372..c65d33e33328 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1837,6 +1837,7 @@ L: linux-amlogic@lists.infradead.org S: Maintained F: Documentation/devicetree/bindings/clock/amlogic* F: drivers/clk/meson/ +F: include/dt-bindings/clock/amlogic* F: include/dt-bindings/clock/gxbb* F: include/dt-bindings/clock/meson* diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig index fc002c155bc3..a663c90a3f3b 100644 --- a/drivers/clk/meson/Kconfig +++ b/drivers/clk/meson/Kconfig @@ -115,4 +115,17 @@ config COMMON_CLK_G12A help Support for the clock controller on Amlogic S905D2, S905X2 and S905Y2 devices, aka g12a. Say Y if you want peripherals to work. + +config COMMON_CLK_S4_PLL + tristate "S4 SoC PLL clock controllers support" + depends on ARM64 + default y + select COMMON_CLK_MESON_MPLL + select COMMON_CLK_MESON_PLL + select COMMON_CLK_MESON_REGMAP + help + Support for the pll clock controller on Amlogic S805X2 and S905Y4 devices, + aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229. + Say Y if you want the board to work, because plls are the parent of most + peripherals. endmenu diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile index 6eca2a406ee3..376f49cc13f1 100644 --- a/drivers/clk/meson/Makefile +++ b/drivers/clk/meson/Makefile @@ -19,3 +19,4 @@ obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o +obj-$(CONFIG_COMMON_CLK_S4_PLL) += s4-pll.o diff --git a/drivers/clk/meson/s4-pll.c b/drivers/clk/meson/s4-pll.c new file mode 100644 index 000000000000..227d4fd7547d --- /dev/null +++ b/drivers/clk/meson/s4-pll.c @@ -0,0 +1,875 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Amlogic Meson-S4 PLL Clock Controller Driver + * + * Copyright (c) 2021 Amlogic, inc. + * Author: Yu Tu <yu.tu@amlogic.com> + */ + +#include <linux/clk-provider.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> + +#include "clk-mpll.h" +#include "clk-pll.h" +#include "clk-regmap.h" +#include "s4-pll.h" + +static DEFINE_SPINLOCK(meson_clk_lock); + +static struct clk_regmap s4_fixed_pll_dco = { + .data = &(struct meson_clk_pll_data){ + .en = { + .reg_off = ANACTRL_FIXPLL_CTRL0, + .shift = 28, + .width = 1, + }, + .m = { + .reg_off = ANACTRL_FIXPLL_CTRL0, + .shift = 0, + .width = 8, + }, + .n = { + .reg_off = ANACTRL_FIXPLL_CTRL0, + .shift = 10, + .width = 5, + }, + .frac = { + .reg_off = ANACTRL_FIXPLL_CTRL1, + .shift = 0, + .width = 17, + }, + .l = { + .reg_off = ANACTRL_FIXPLL_CTRL0, + .shift = 31, + .width = 1, + }, + .rst = { + .reg_off = ANACTRL_FIXPLL_CTRL0, + .shift = 29, + .width = 1, + }, + }, + .hw.init = &(struct clk_init_data){ + .name = "fixed_pll_dco", + .ops = &meson_clk_pll_ro_ops, + .parent_data = (const struct clk_parent_data []) { + { .fw_name = "xtal", } + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_fixed_pll = { + .data = &(struct clk_regmap_div_data){ + .offset = ANACTRL_FIXPLL_CTRL0, + .shift = 16, + .width = 2, + .flags = CLK_DIVIDER_POWER_OF_TWO, + }, + .hw.init = &(struct clk_init_data){ + .name = "fixed_pll", + .ops = &clk_regmap_divider_ro_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_fixed_pll_dco.hw + }, + .num_parents = 1, + /* + * This clock won't ever change at runtime so + * CLK_SET_RATE_PARENT is not required + */ + }, +}; + +static struct clk_fixed_factor s4_fclk_div2_div = { + .mult = 1, + .div = 2, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div2_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { &s4_fixed_pll.hw }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_fclk_div2 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL1, + .bit_idx = 24, + }, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div2", + .ops = &clk_regmap_gate_ro_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_fclk_div2_div.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_fixed_factor s4_fclk_div3_div = { + .mult = 1, + .div = 3, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div3_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { &s4_fixed_pll.hw }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_fclk_div3 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL1, + .bit_idx = 20, + }, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div3", + .ops = &clk_regmap_gate_ro_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_fclk_div3_div.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_fixed_factor s4_fclk_div4_div = { + .mult = 1, + .div = 4, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div4_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { &s4_fixed_pll.hw }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_fclk_div4 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL1, + .bit_idx = 21, + }, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div4", + .ops = &clk_regmap_gate_ro_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_fclk_div4_div.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_fixed_factor s4_fclk_div5_div = { + .mult = 1, + .div = 5, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div5_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { &s4_fixed_pll.hw }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_fclk_div5 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL1, + .bit_idx = 22, + }, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div5", + .ops = &clk_regmap_gate_ro_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_fclk_div5_div.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_fixed_factor s4_fclk_div7_div = { + .mult = 1, + .div = 7, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div7_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { &s4_fixed_pll.hw }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_fclk_div7 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL1, + .bit_idx = 23, + }, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div7", + .ops = &clk_regmap_gate_ro_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_fclk_div7_div.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_fixed_factor s4_fclk_div2p5_div = { + .mult = 2, + .div = 5, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div2p5_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_fixed_pll.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_fclk_div2p5 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_FIXPLL_CTRL1, + .bit_idx = 25, + }, + .hw.init = &(struct clk_init_data){ + .name = "fclk_div2p5", + .ops = &clk_regmap_gate_ro_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_fclk_div2p5_div.hw + }, + .num_parents = 1, + }, +}; + +static const struct pll_mult_range s4_gp0_pll_mult_range = { + .min = 125, + .max = 250, +}; + +/* + * Internal gp0 pll emulation configuration parameters + */ +static const struct reg_sequence s4_gp0_init_regs[] = { + { .reg = ANACTRL_GP0PLL_CTRL1, .def = 0x00000000 }, + { .reg = ANACTRL_GP0PLL_CTRL2, .def = 0x00000000 }, + { .reg = ANACTRL_GP0PLL_CTRL3, .def = 0x48681c00 }, + { .reg = ANACTRL_GP0PLL_CTRL4, .def = 0x88770290 }, + { .reg = ANACTRL_GP0PLL_CTRL5, .def = 0x39272000 }, + { .reg = ANACTRL_GP0PLL_CTRL6, .def = 0x56540000 } +}; + +static struct clk_regmap s4_gp0_pll_dco = { + .data = &(struct meson_clk_pll_data){ + .en = { + .reg_off = ANACTRL_GP0PLL_CTRL0, + .shift = 28, + .width = 1, + }, + .m = { + .reg_off = ANACTRL_GP0PLL_CTRL0, + .shift = 0, + .width = 8, + }, + .n = { + .reg_off = ANACTRL_GP0PLL_CTRL0, + .shift = 10, + .width = 5, + }, + .frac = { + .reg_off = ANACTRL_GP0PLL_CTRL1, + .shift = 0, + .width = 17, + }, + .l = { + .reg_off = ANACTRL_GP0PLL_CTRL0, + .shift = 31, + .width = 1, + }, + .rst = { + .reg_off = ANACTRL_GP0PLL_CTRL0, + .shift = 29, + .width = 1, + }, + .range = &s4_gp0_pll_mult_range, + .init_regs = s4_gp0_init_regs, + .init_count = ARRAY_SIZE(s4_gp0_init_regs), + }, + .hw.init = &(struct clk_init_data){ + .name = "gp0_pll_dco", + .ops = &meson_clk_pll_ops, + .parent_data = (const struct clk_parent_data []) { + { .fw_name = "xtal", } + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_gp0_pll = { + .data = &(struct clk_regmap_div_data){ + .offset = ANACTRL_GP0PLL_CTRL0, + .shift = 16, + .width = 3, + .flags = (CLK_DIVIDER_POWER_OF_TWO | + CLK_DIVIDER_ROUND_CLOSEST), + }, + .hw.init = &(struct clk_init_data){ + .name = "gp0_pll", + .ops = &clk_regmap_divider_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_gp0_pll_dco.hw + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + }, +}; + +/* + * Internal hifi pll emulation configuration parameters + */ +static const struct reg_sequence s4_hifi_init_regs[] = { + { .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x00010e56 }, + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00000000 }, + { .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x6a285c00 }, + { .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x65771290 }, + { .reg = ANACTRL_HIFIPLL_CTRL5, .def = 0x39272000 }, + { .reg = ANACTRL_HIFIPLL_CTRL6, .def = 0x56540000 } +}; + +static struct clk_regmap s4_hifi_pll_dco = { + .data = &(struct meson_clk_pll_data){ + .en = { + .reg_off = ANACTRL_HIFIPLL_CTRL0, + .shift = 28, + .width = 1, + }, + .m = { + .reg_off = ANACTRL_HIFIPLL_CTRL0, + .shift = 0, + .width = 8, + }, + .n = { + .reg_off = ANACTRL_HIFIPLL_CTRL0, + .shift = 10, + .width = 5, + }, + .frac = { + .reg_off = ANACTRL_HIFIPLL_CTRL1, + .shift = 0, + .width = 17, + }, + .l = { + .reg_off = ANACTRL_HIFIPLL_CTRL0, + .shift = 31, + .width = 1, + }, + .rst = { + .reg_off = ANACTRL_HIFIPLL_CTRL0, + .shift = 29, + .width = 1, + }, + .range = &s4_gp0_pll_mult_range, + .init_regs = s4_hifi_init_regs, + .init_count = ARRAY_SIZE(s4_hifi_init_regs), + .flags = CLK_MESON_PLL_ROUND_CLOSEST, + }, + .hw.init = &(struct clk_init_data){ + .name = "hifi_pll_dco", + .ops = &meson_clk_pll_ops, + .parent_data = (const struct clk_parent_data []) { + { .fw_name = "xtal", } + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_hifi_pll = { + .data = &(struct clk_regmap_div_data){ + .offset = ANACTRL_HIFIPLL_CTRL0, + .shift = 16, + .width = 2, + .flags = (CLK_DIVIDER_POWER_OF_TWO | + CLK_DIVIDER_ROUND_CLOSEST), + }, + .hw.init = &(struct clk_init_data){ + .name = "hifi_pll", + .ops = &clk_regmap_divider_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_hifi_pll_dco.hw + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + }, +}; + +static struct clk_regmap s4_hdmi_pll_dco = { + .data = &(struct meson_clk_pll_data){ + .en = { + .reg_off = ANACTRL_HDMIPLL_CTRL0, + .shift = 28, + .width = 1, + }, + .m = { + .reg_off = ANACTRL_HDMIPLL_CTRL0, + .shift = 0, + .width = 8, + }, + .n = { + .reg_off = ANACTRL_HDMIPLL_CTRL0, + .shift = 10, + .width = 5, + }, + .frac = { + .reg_off = ANACTRL_HDMIPLL_CTRL1, + .shift = 0, + .width = 17, + }, + .l = { + .reg_off = ANACTRL_HDMIPLL_CTRL0, + .shift = 31, + .width = 1, + }, + .rst = { + .reg_off = ANACTRL_HDMIPLL_CTRL0, + .shift = 29, + .width = 1, + }, + .range = &s4_gp0_pll_mult_range, + }, + .hw.init = &(struct clk_init_data){ + .name = "hdmi_pll_dco", + .ops = &meson_clk_pll_ops, + .parent_data = (const struct clk_parent_data []) { + { .fw_name = "xtal", } + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_hdmi_pll_od = { + .data = &(struct clk_regmap_div_data){ + .offset = ANACTRL_HDMIPLL_CTRL0, + .shift = 16, + .width = 4, + .flags = CLK_DIVIDER_POWER_OF_TWO, + }, + .hw.init = &(struct clk_init_data){ + .name = "hdmi_pll_od", + .ops = &clk_regmap_divider_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_hdmi_pll_dco.hw + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + }, +}; + +static struct clk_regmap s4_hdmi_pll = { + .data = &(struct clk_regmap_div_data){ + .offset = ANACTRL_HDMIPLL_CTRL0, + .shift = 20, + .width = 2, + .flags = CLK_DIVIDER_POWER_OF_TWO, + }, + .hw.init = &(struct clk_init_data){ + .name = "hdmi_pll", + .ops = &clk_regmap_divider_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_hdmi_pll_od.hw + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + }, +}; + +static struct clk_fixed_factor s4_mpll_50m_div = { + .mult = 1, + .div = 80, + .hw.init = &(struct clk_init_data){ + .name = "mpll_50m_div", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_fixed_pll_dco.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_mpll_50m = { + .data = &(struct clk_regmap_mux_data){ + .offset = ANACTRL_FIXPLL_CTRL3, + .mask = 0x1, + .shift = 5, + }, + .hw.init = &(struct clk_init_data){ + .name = "mpll_50m", + .ops = &clk_regmap_mux_ro_ops, + .parent_data = (const struct clk_parent_data []) { + { .fw_name = "xtal", }, + { .hw = &s4_mpll_50m_div.hw }, + }, + .num_parents = 2, + }, +}; + +static struct clk_fixed_factor s4_mpll_prediv = { + .mult = 1, + .div = 2, + .hw.init = &(struct clk_init_data){ + .name = "mpll_prediv", + .ops = &clk_fixed_factor_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_fixed_pll_dco.hw + }, + .num_parents = 1, + }, +}; + +static const struct reg_sequence s4_mpll0_init_regs[] = { + { .reg = ANACTRL_MPLL_CTRL2, .def = 0x40000033 } +}; + +static struct clk_regmap s4_mpll0_div = { + .data = &(struct meson_clk_mpll_data){ + .sdm = { + .reg_off = ANACTRL_MPLL_CTRL1, + .shift = 0, + .width = 14, + }, + .sdm_en = { + .reg_off = ANACTRL_MPLL_CTRL1, + .shift = 30, + .width = 1, + }, + .n2 = { + .reg_off = ANACTRL_MPLL_CTRL1, + .shift = 20, + .width = 9, + }, + .ssen = { + .reg_off = ANACTRL_MPLL_CTRL1, + .shift = 29, + .width = 1, + }, + .lock = &meson_clk_lock, + .init_regs = s4_mpll0_init_regs, + .init_count = ARRAY_SIZE(s4_mpll0_init_regs), + }, + .hw.init = &(struct clk_init_data){ + .name = "mpll0_div", + .ops = &meson_clk_mpll_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_mpll_prediv.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_mpll0 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_MPLL_CTRL1, + .bit_idx = 31, + }, + .hw.init = &(struct clk_init_data){ + .name = "mpll0", + .ops = &clk_regmap_gate_ops, + .parent_hws = (const struct clk_hw *[]) { &s4_mpll0_div.hw }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + }, +}; + +static const struct reg_sequence s4_mpll1_init_regs[] = { + { .reg = ANACTRL_MPLL_CTRL4, .def = 0x40000033 } +}; + +static struct clk_regmap s4_mpll1_div = { + .data = &(struct meson_clk_mpll_data){ + .sdm = { + .reg_off = ANACTRL_MPLL_CTRL3, + .shift = 0, + .width = 14, + }, + .sdm_en = { + .reg_off = ANACTRL_MPLL_CTRL3, + .shift = 30, + .width = 1, + }, + .n2 = { + .reg_off = ANACTRL_MPLL_CTRL3, + .shift = 20, + .width = 9, + }, + .ssen = { + .reg_off = ANACTRL_MPLL_CTRL3, + .shift = 29, + .width = 1, + }, + .lock = &meson_clk_lock, + .init_regs = s4_mpll1_init_regs, + .init_count = ARRAY_SIZE(s4_mpll1_init_regs), + }, + .hw.init = &(struct clk_init_data){ + .name = "mpll1_div", + .ops = &meson_clk_mpll_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_mpll_prediv.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_mpll1 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_MPLL_CTRL3, + .bit_idx = 31, + }, + .hw.init = &(struct clk_init_data){ + .name = "mpll1", + .ops = &clk_regmap_gate_ops, + .parent_hws = (const struct clk_hw *[]) { &s4_mpll1_div.hw }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + }, +}; + +static const struct reg_sequence s4_mpll2_init_regs[] = { + { .reg = ANACTRL_MPLL_CTRL6, .def = 0x40000033 } +}; + +static struct clk_regmap s4_mpll2_div = { + .data = &(struct meson_clk_mpll_data){ + .sdm = { + .reg_off = ANACTRL_MPLL_CTRL5, + .shift = 0, + .width = 14, + }, + .sdm_en = { + .reg_off = ANACTRL_MPLL_CTRL5, + .shift = 30, + .width = 1, + }, + .n2 = { + .reg_off = ANACTRL_MPLL_CTRL5, + .shift = 20, + .width = 9, + }, + .ssen = { + .reg_off = ANACTRL_MPLL_CTRL5, + .shift = 29, + .width = 1, + }, + .lock = &meson_clk_lock, + .init_regs = s4_mpll2_init_regs, + .init_count = ARRAY_SIZE(s4_mpll2_init_regs), + }, + .hw.init = &(struct clk_init_data){ + .name = "mpll2_div", + .ops = &meson_clk_mpll_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_mpll_prediv.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_mpll2 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_MPLL_CTRL5, + .bit_idx = 31, + }, + .hw.init = &(struct clk_init_data){ + .name = "mpll2", + .ops = &clk_regmap_gate_ops, + .parent_hws = (const struct clk_hw *[]) { &s4_mpll2_div.hw }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + }, +}; + +static const struct reg_sequence s4_mpll3_init_regs[] = { + { .reg = ANACTRL_MPLL_CTRL8, .def = 0x40000033 } +}; + +static struct clk_regmap s4_mpll3_div = { + .data = &(struct meson_clk_mpll_data){ + .sdm = { + .reg_off = ANACTRL_MPLL_CTRL7, + .shift = 0, + .width = 14, + }, + .sdm_en = { + .reg_off = ANACTRL_MPLL_CTRL7, + .shift = 30, + .width = 1, + }, + .n2 = { + .reg_off = ANACTRL_MPLL_CTRL7, + .shift = 20, + .width = 9, + }, + .ssen = { + .reg_off = ANACTRL_MPLL_CTRL7, + .shift = 29, + .width = 1, + }, + .lock = &meson_clk_lock, + .init_regs = s4_mpll3_init_regs, + .init_count = ARRAY_SIZE(s4_mpll3_init_regs), + }, + .hw.init = &(struct clk_init_data){ + .name = "mpll3_div", + .ops = &meson_clk_mpll_ops, + .parent_hws = (const struct clk_hw *[]) { + &s4_mpll_prediv.hw + }, + .num_parents = 1, + }, +}; + +static struct clk_regmap s4_mpll3 = { + .data = &(struct clk_regmap_gate_data){ + .offset = ANACTRL_MPLL_CTRL7, + .bit_idx = 31, + }, + .hw.init = &(struct clk_init_data){ + .name = "mpll3", + .ops = &clk_regmap_gate_ops, + .parent_hws = (const struct clk_hw *[]) { &s4_mpll3_div.hw }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + }, +}; + +/* Array of all clocks provided by this provider */ +static struct clk_hw_onecell_data s4_pll_hw_onecell_data = { + .hws = { + [CLKID_FIXED_PLL_DCO] = &s4_fixed_pll_dco.hw, + [CLKID_FIXED_PLL] = &s4_fixed_pll.hw, + [CLKID_FCLK_DIV2_DIV] = &s4_fclk_div2_div.hw, + [CLKID_FCLK_DIV2] = &s4_fclk_div2.hw, + [CLKID_FCLK_DIV3_DIV] = &s4_fclk_div3_div.hw, + [CLKID_FCLK_DIV3] = &s4_fclk_div3.hw, + [CLKID_FCLK_DIV4_DIV] = &s4_fclk_div4_div.hw, + [CLKID_FCLK_DIV4] = &s4_fclk_div4.hw, + [CLKID_FCLK_DIV5_DIV] = &s4_fclk_div5_div.hw, + [CLKID_FCLK_DIV5] = &s4_fclk_div5.hw, + [CLKID_FCLK_DIV7_DIV] = &s4_fclk_div7_div.hw, + [CLKID_FCLK_DIV7] = &s4_fclk_div7.hw, + [CLKID_FCLK_DIV2P5_DIV] = &s4_fclk_div2p5_div.hw, + [CLKID_FCLK_DIV2P5] = &s4_fclk_div2p5.hw, + [CLKID_GP0_PLL_DCO] = &s4_gp0_pll_dco.hw, + [CLKID_GP0_PLL] = &s4_gp0_pll.hw, + [CLKID_HIFI_PLL_DCO] = &s4_hifi_pll_dco.hw, + [CLKID_HIFI_PLL] = &s4_hifi_pll.hw, + [CLKID_HDMI_PLL_DCO] = &s4_hdmi_pll_dco.hw, + [CLKID_HDMI_PLL_OD] = &s4_hdmi_pll_od.hw, + [CLKID_HDMI_PLL] = &s4_hdmi_pll.hw, + [CLKID_MPLL_50M_DIV] = &s4_mpll_50m_div.hw, + [CLKID_MPLL_50M] = &s4_mpll_50m.hw, + [CLKID_MPLL_PREDIV] = &s4_mpll_prediv.hw, + [CLKID_MPLL0_DIV] = &s4_mpll0_div.hw, + [CLKID_MPLL0] = &s4_mpll0.hw, + [CLKID_MPLL1_DIV] = &s4_mpll1_div.hw, + [CLKID_MPLL1] = &s4_mpll1.hw, + [CLKID_MPLL2_DIV] = &s4_mpll2_div.hw, + [CLKID_MPLL2] = &s4_mpll2.hw, + [CLKID_MPLL3_DIV] = &s4_mpll3_div.hw, + [CLKID_MPLL3] = &s4_mpll3.hw, + [NR_PLL_CLKS] = NULL + }, + .num = NR_PLL_CLKS, +}; + +static struct clk_regmap *const s4_pll_clk_regmaps[] = { + &s4_fixed_pll_dco, + &s4_fixed_pll, + &s4_fclk_div2, + &s4_fclk_div3, + &s4_fclk_div4, + &s4_fclk_div5, + &s4_fclk_div7, + &s4_fclk_div2p5, + &s4_gp0_pll_dco, + &s4_gp0_pll, + &s4_hifi_pll_dco, + &s4_hifi_pll, + &s4_hdmi_pll_dco, + &s4_hdmi_pll_od, + &s4_hdmi_pll, + &s4_mpll_50m, + &s4_mpll0_div, + &s4_mpll0, + &s4_mpll1_div, + &s4_mpll1, + &s4_mpll2_div, + &s4_mpll2, + &s4_mpll3_div, + &s4_mpll3, +}; + +static const struct reg_sequence s4_init_regs[] = { + { .reg = ANACTRL_MPLL_CTRL0, .def = 0x00000543 }, +}; + +static struct regmap_config clkc_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, +}; + +static int meson_s4_pll_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct regmap *regmap; + void __iomem *base; + int ret, i; + + base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(base)) + return PTR_ERR(base); + + regmap = devm_regmap_init_mmio(dev, base, &clkc_regmap_config); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + ret = regmap_multi_reg_write(regmap, s4_init_regs, ARRAY_SIZE(s4_init_regs)); + if (ret) { + dev_err(dev, "Failed to init registers\n"); + return ret; + } + + /* Populate regmap for the regmap backed clocks */ + for (i = 0; i < ARRAY_SIZE(s4_pll_clk_regmaps); i++) + s4_pll_clk_regmaps[i]->map = regmap; + + for (i = 0; i < s4_pll_hw_onecell_data.num; i++) { + /* array might be sparse */ + if (!s4_pll_hw_onecell_data.hws[i]) + continue; + + ret = devm_clk_hw_register(dev, s4_pll_hw_onecell_data.hws[i]); + if (ret) { + dev_err(dev, "Clock registration failed\n"); + return ret; + } + } + + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, + &s4_pll_hw_onecell_data); +} + +static const struct of_device_id clkc_match_table[] = { + { + .compatible = "amlogic,s4-pll-clkc", + }, + {} +}; + +static struct platform_driver s4_driver = { + .probe = meson_s4_pll_probe, + .driver = { + .name = "s4-pll-clkc", + .of_match_table = clkc_match_table, + }, +}; + +module_platform_driver(s4_driver); +MODULE_LICENSE("GPL"); diff --git a/drivers/clk/meson/s4-pll.h b/drivers/clk/meson/s4-pll.h new file mode 100644 index 000000000000..332f2d7b94b4 --- /dev/null +++ b/drivers/clk/meson/s4-pll.h @@ -0,0 +1,88 @@ +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ +/* + * Copyright (c) 2021 Amlogic, inc. + * Author: Yu Tu <yu.tu@amlogic.com> + */ + +#ifndef __MESON_S4_PLL_H__ +#define __MESON_S4_PLL_H__ + +/* ANA_CTRL - Registers + * REG_BASE: REGISTER_BASE_ADDR = 0xfe008000 + */ +#define ANACTRL_FIXPLL_CTRL0 0x040 +#define ANACTRL_FIXPLL_CTRL1 0x044 +#define ANACTRL_FIXPLL_CTRL2 0x048 +#define ANACTRL_FIXPLL_CTRL3 0x04c +#define ANACTRL_FIXPLL_CTRL4 0x050 +#define ANACTRL_FIXPLL_CTRL5 0x054 +#define ANACTRL_FIXPLL_CTRL6 0x058 +#define ANACTRL_FIXPLL_STS 0x05c +#define ANACTRL_GP0PLL_CTRL0 0x080 +#define ANACTRL_GP0PLL_CTRL1 0x084 +#define ANACTRL_GP0PLL_CTRL2 0x088 +#define ANACTRL_GP0PLL_CTRL3 0x08c +#define ANACTRL_GP0PLL_CTRL4 0x090 +#define ANACTRL_GP0PLL_CTRL5 0x094 +#define ANACTRL_GP0PLL_CTRL6 0x098 +#define ANACTRL_GP0PLL_STS 0x09c +#define ANACTRL_HIFIPLL_CTRL0 0x100 +#define ANACTRL_HIFIPLL_CTRL1 0x104 +#define ANACTRL_HIFIPLL_CTRL2 0x108 +#define ANACTRL_HIFIPLL_CTRL3 0x10c +#define ANACTRL_HIFIPLL_CTRL4 0x110 +#define ANACTRL_HIFIPLL_CTRL5 0x114 +#define ANACTRL_HIFIPLL_CTRL6 0x118 +#define ANACTRL_HIFIPLL_STS 0x11c +#define ANACTRL_MPLL_CTRL0 0x180 +#define ANACTRL_MPLL_CTRL1 0x184 +#define ANACTRL_MPLL_CTRL2 0x188 +#define ANACTRL_MPLL_CTRL3 0x18c +#define ANACTRL_MPLL_CTRL4 0x190 +#define ANACTRL_MPLL_CTRL5 0x194 +#define ANACTRL_MPLL_CTRL6 0x198 +#define ANACTRL_MPLL_CTRL7 0x19c +#define ANACTRL_MPLL_CTRL8 0x1a0 +#define ANACTRL_MPLL_STS 0x1a4 +#define ANACTRL_HDMIPLL_CTRL0 0x1c0 +#define ANACTRL_HDMIPLL_CTRL1 0x1c4 +#define ANACTRL_HDMIPLL_CTRL2 0x1c8 +#define ANACTRL_HDMIPLL_CTRL3 0x1cc +#define ANACTRL_HDMIPLL_CTRL4 0x1d0 +#define ANACTRL_HDMIPLL_CTRL5 0x1d4 +#define ANACTRL_HDMIPLL_CTRL6 0x1d8 +#define ANACTRL_HDMIPLL_STS 0x1dc +#define ANACTRL_HDMIPLL_VLOCK 0x1e4 + +/* + * CLKID index values + * + * These indices are entirely contrived and do not map onto the hardware. + * It has now been decided to expose everything by default in the DT header: + * include/dt-bindings/clock/axg-clkc.h. Only the clocks ids we don't want + * to expose, such as the internal muxes and dividers of composite clocks, + * will remain defined here. + */ +#define CLKID_FIXED_PLL_DCO 0 +#define CLKID_FCLK_DIV2_DIV 2 +#define CLKID_FCLK_DIV3_DIV 4 +#define CLKID_FCLK_DIV4_DIV 6 +#define CLKID_FCLK_DIV5_DIV 8 +#define CLKID_FCLK_DIV7_DIV 10 +#define CLKID_FCLK_DIV2P5_DIV 12 +#define CLKID_GP0_PLL_DCO 14 +#define CLKID_HIFI_PLL_DCO 16 +#define CLKID_HDMI_PLL_DCO 18 +#define CLKID_HDMI_PLL_OD 19 +#define CLKID_MPLL_50M_DIV 21 +#define CLKID_MPLL_PREDIV 23 +#define CLKID_MPLL0_DIV 24 +#define CLKID_MPLL1_DIV 26 +#define CLKID_MPLL2_DIV 28 +#define CLKID_MPLL3_DIV 30 + +#define NR_PLL_CLKS 32 +/* include the CLKIDs that have been made part of the DT binding */ +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> + +#endif /* __MESON_S4_PLL_H__ */ diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h new file mode 100644 index 000000000000..345f87023886 --- /dev/null +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h @@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ +/* + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. + * Author: Yu Tu <yu.tu@amlogic.com> + */ + +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H + +/* + * CLKID index values + */ + +#define CLKID_FIXED_PLL 1 +#define CLKID_FCLK_DIV2 3 +#define CLKID_FCLK_DIV3 5 +#define CLKID_FCLK_DIV4 7 +#define CLKID_FCLK_DIV5 9 +#define CLKID_FCLK_DIV7 11 +#define CLKID_FCLK_DIV2P5 13 +#define CLKID_GP0_PLL 15 +#define CLKID_HIFI_PLL 17 +#define CLKID_HDMI_PLL 20 +#define CLKID_MPLL_50M 22 +#define CLKID_MPLL0 25 +#define CLKID_MPLL1 27 +#define CLKID_MPLL2 29 +#define CLKID_MPLL3 31 + +#endif /* _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H */ -- 2.33.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-23 2:13 ` [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings Yu Tu @ 2022-11-23 10:08 ` Krzysztof Kozlowski 2022-11-23 11:16 ` Yu Tu 0 siblings, 1 reply; 34+ messages in thread From: Krzysztof Kozlowski @ 2022-11-23 10:08 UTC (permalink / raw) To: Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 03:13, Yu Tu wrote: > Add the S4 PLL clock controller found and bindings in the s4 SoC family. > > Signed-off-by: Yu Tu <yu.tu@amlogic.com> > --- > .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + This is v5 and still bindings are here? Bindings are always separate patches. Use subject prefixes matching the subsystem (git log --oneline -- ...). And this was split, wasn't it? What happened here?!? > MAINTAINERS | 1 + > drivers/clk/meson/Kconfig | 13 + > drivers/clk/meson/Makefile | 1 + > drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ > drivers/clk/meson/s4-pll.h | 88 ++ > .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + > 7 files changed, 1059 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml > create mode 100644 drivers/clk/meson/s4-pll.c > create mode 100644 drivers/clk/meson/s4-pll.h > create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h > > diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml > new file mode 100644 > index 000000000000..fd517e8ef14f > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml > @@ -0,0 +1,51 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Amlogic Meson S serials PLL Clock Controller > + > +maintainers: > + - Neil Armstrong <narmstrong@baylibre.com> > + - Jerome Brunet <jbrunet@baylibre.com> > + - Yu Tu <yu.hu@amlogic.com> > + One blank line. > + > +properties: > + compatible: > + const: amlogic,s4-pll-clkc > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + items: > + - const: xtal > + > + "#clock-cells": > + const: 1 > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - "#clock-cells" > + > +additionalProperties: false > + > +examples: > + - | > + clkc_pll: clock-controller@fe008000 { > + compatible = "amlogic,s4-pll-clkc"; > + reg = <0xfe008000 0x1e8>; > + clocks = <&xtal>; > + clock-names = "xtal"; > + #clock-cells = <1>; > + }; > +#endif /* __MESON_S4_PLL_H__ */ > diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h > new file mode 100644 > index 000000000000..345f87023886 > --- /dev/null > +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h This belongs to bindings patch, not driver. > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ > +/* > + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. > + * Author: Yu Tu <yu.tu@amlogic.com> > + */ > + > +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H > +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H > + > +/* > + * CLKID index values > + */ > + > +#define CLKID_FIXED_PLL 1 > +#define CLKID_FCLK_DIV2 3 Indexes start from 0 and are incremented by 1. Not by 2. NAK. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-23 10:08 ` Krzysztof Kozlowski @ 2022-11-23 11:16 ` Yu Tu 2022-11-23 13:05 ` Krzysztof Kozlowski 2022-11-23 13:23 ` Neil Armstrong 0 siblings, 2 replies; 34+ messages in thread From: Yu Tu @ 2022-11-23 11:16 UTC (permalink / raw) To: Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang Hi Krzysztof, Thank you for your reply. On 2022/11/23 18:08, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 03:13, Yu Tu wrote: >> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> --- >> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + > > This is v5 and still bindings are here? Bindings are always separate > patches. Use subject prefixes matching the subsystem (git log --oneline > -- ...). > > And this was split, wasn't it? What happened here?!? Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history. https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ > > >> MAINTAINERS | 1 + >> drivers/clk/meson/Kconfig | 13 + >> drivers/clk/meson/Makefile | 1 + >> drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ >> drivers/clk/meson/s4-pll.h | 88 ++ >> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >> 7 files changed, 1059 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >> create mode 100644 drivers/clk/meson/s4-pll.c >> create mode 100644 drivers/clk/meson/s4-pll.h >> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >> >> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >> new file mode 100644 >> index 000000000000..fd517e8ef14f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >> @@ -0,0 +1,51 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Amlogic Meson S serials PLL Clock Controller >> + >> +maintainers: >> + - Neil Armstrong <narmstrong@baylibre.com> >> + - Jerome Brunet <jbrunet@baylibre.com> >> + - Yu Tu <yu.hu@amlogic.com> >> + > One blank line. I will delete this, on next version patch. > >> + >> +properties: >> + compatible: >> + const: amlogic,s4-pll-clkc >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> + clock-names: >> + items: >> + - const: xtal >> + >> + "#clock-cells": >> + const: 1 >> + >> +required: >> + - compatible >> + - reg >> + - clocks >> + - clock-names >> + - "#clock-cells" >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + clkc_pll: clock-controller@fe008000 { >> + compatible = "amlogic,s4-pll-clkc"; >> + reg = <0xfe008000 0x1e8>; >> + clocks = <&xtal>; >> + clock-names = "xtal"; >> + #clock-cells = <1>; >> + }; > > >> +#endif /* __MESON_S4_PLL_H__ */ >> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >> new file mode 100644 >> index 000000000000..345f87023886 >> --- /dev/null >> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h > > This belongs to bindings patch, not driver. > >> @@ -0,0 +1,30 @@ >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >> +/* >> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >> + * Author: Yu Tu <yu.tu@amlogic.com> >> + */ >> + >> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >> + >> +/* >> + * CLKID index values >> + */ >> + >> +#define CLKID_FIXED_PLL 1 >> +#define CLKID_FCLK_DIV2 3 > > Indexes start from 0 and are incremented by 1. Not by 2. > > NAK. I remember Jerome discussing this with you.You can look at this submission history. https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ > > Best regards, > Krzysztof > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-23 11:16 ` Yu Tu @ 2022-11-23 13:05 ` Krzysztof Kozlowski 2022-11-23 13:23 ` Neil Armstrong 1 sibling, 0 replies; 34+ messages in thread From: Krzysztof Kozlowski @ 2022-11-23 13:05 UTC (permalink / raw) To: Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 12:16, Yu Tu wrote: > Hi Krzysztof, > Thank you for your reply. > > On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >> [ EXTERNAL EMAIL ] >> >> On 23/11/2022 03:13, Yu Tu wrote: >>> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >> >> This is v5 and still bindings are here? Bindings are always separate >> patches. Use subject prefixes matching the subsystem (git log --oneline >> -- ...). >> >> And this was split, wasn't it? What happened here?!? > > Put bindings and clock driver patch together from Jerome. Maybe you can > read this chat history. > https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ Link does not explain me anything. It mentions series, which is totally different than mixing it one patch! Anyway you should have warnings from checkpatch. Bindings are always separate patches. (...) >> >>> + >>> +properties: >>> + compatible: >>> + const: amlogic,s4-pll-clkc >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> + clock-names: >>> + items: >>> + - const: xtal >>> + >>> + "#clock-cells": >>> + const: 1 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - clocks >>> + - clock-names >>> + - "#clock-cells" >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + clkc_pll: clock-controller@fe008000 { >>> + compatible = "amlogic,s4-pll-clkc"; >>> + reg = <0xfe008000 0x1e8>; >>> + clocks = <&xtal>; >>> + clock-names = "xtal"; >>> + #clock-cells = <1>; >>> + }; >> >> >>> +#endif /* __MESON_S4_PLL_H__ */ >>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>> new file mode 100644 >>> index 000000000000..345f87023886 >>> --- /dev/null >>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >> >> This belongs to bindings patch, not driver. >> >>> @@ -0,0 +1,30 @@ >>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>> +/* >>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>> + * Author: Yu Tu <yu.tu@amlogic.com> >>> + */ >>> + >>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>> + >>> +/* >>> + * CLKID index values >>> + */ >>> + >>> +#define CLKID_FIXED_PLL 1 >>> +#define CLKID_FCLK_DIV2 3 >> >> Indexes start from 0 and are incremented by 1. Not by 2. >> >> NAK. > > I remember Jerome discussing this with you.You can look at this > submission history. > https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ You pointed to my arguments, so what is this proving? That you ignored feedback? Or was there some other mail? Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-23 11:16 ` Yu Tu 2022-11-23 13:05 ` Krzysztof Kozlowski @ 2022-11-23 13:23 ` Neil Armstrong 2022-11-23 13:53 ` Krzysztof Kozlowski 2022-11-23 13:54 ` Yu Tu 1 sibling, 2 replies; 34+ messages in thread From: Neil Armstrong @ 2022-11-23 13:23 UTC (permalink / raw) To: Yu Tu, Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang Hi, On 23/11/2022 12:16, Yu Tu wrote: > Hi Krzysztof, > Thank you for your reply. > > On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >> [ EXTERNAL EMAIL ] >> >> On 23/11/2022 03:13, Yu Tu wrote: >>> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >> >> This is v5 and still bindings are here? Bindings are always separate >> patches. Use subject prefixes matching the subsystem (git log --oneline >> -- ...). >> >> And this was split, wasn't it? What happened here?!? > > Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history. > https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ Jerome was asking you to send 2 patchsets, one with : - bindings in separate patches - drivers in separate patches and a second with DT changes. Then when the bindings + clocks patches are merged, a pull request of the bindings can be done to me so I can merge it with DT. > >> >> >>> MAINTAINERS | 1 + >>> drivers/clk/meson/Kconfig | 13 + >>> drivers/clk/meson/Makefile | 1 + >>> drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ >>> drivers/clk/meson/s4-pll.h | 88 ++ >>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>> 7 files changed, 1059 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>> create mode 100644 drivers/clk/meson/s4-pll.c >>> create mode 100644 drivers/clk/meson/s4-pll.h >>> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>> >>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>> new file mode 100644 >>> index 000000000000..fd517e8ef14f >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>> @@ -0,0 +1,51 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Amlogic Meson S serials PLL Clock Controller >>> + >>> +maintainers: >>> + - Neil Armstrong <narmstrong@baylibre.com> >>> + - Jerome Brunet <jbrunet@baylibre.com> >>> + - Yu Tu <yu.hu@amlogic.com> >>> + >> One blank line. > > I will delete this, on next version patch. > >> >>> + >>> +properties: >>> + compatible: >>> + const: amlogic,s4-pll-clkc >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> + clock-names: >>> + items: >>> + - const: xtal >>> + >>> + "#clock-cells": >>> + const: 1 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - clocks >>> + - clock-names >>> + - "#clock-cells" >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + clkc_pll: clock-controller@fe008000 { >>> + compatible = "amlogic,s4-pll-clkc"; >>> + reg = <0xfe008000 0x1e8>; >>> + clocks = <&xtal>; >>> + clock-names = "xtal"; >>> + #clock-cells = <1>; >>> + }; >> >> >>> +#endif /* __MESON_S4_PLL_H__ */ >>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>> new file mode 100644 >>> index 000000000000..345f87023886 >>> --- /dev/null >>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >> >> This belongs to bindings patch, not driver. >> >>> @@ -0,0 +1,30 @@ >>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>> +/* >>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>> + * Author: Yu Tu <yu.tu@amlogic.com> >>> + */ >>> + >>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>> + >>> +/* >>> + * CLKID index values >>> + */ >>> + >>> +#define CLKID_FIXED_PLL 1 >>> +#define CLKID_FCLK_DIV2 3 >> >> Indexes start from 0 and are incremented by 1. Not by 2. >> >> NAK. > > I remember Jerome discussing this with you.You can look at this submission history. > https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ Historically we did that by only exposing part of the numbers, controlling which clocks were part of the bindings. But it seems this doesn't make sens anymore, maybe it would be time to put all the clock ids in the bindings for this new SoC and break with the previous strategy. Neil > >> >> Best regards, >> Krzysztof >> >> . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-23 13:23 ` Neil Armstrong @ 2022-11-23 13:53 ` Krzysztof Kozlowski 2022-11-25 9:23 ` Jerome Brunet 2022-11-23 13:54 ` Yu Tu 1 sibling, 1 reply; 34+ messages in thread From: Krzysztof Kozlowski @ 2022-11-23 13:53 UTC (permalink / raw) To: neil.armstrong, Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 14:23, Neil Armstrong wrote: > Hi, > > On 23/11/2022 12:16, Yu Tu wrote: >> Hi Krzysztof, >> Thank you for your reply. >> >> On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On 23/11/2022 03:13, Yu Tu wrote: >>>> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >>>> >>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>> --- >>>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >>> >>> This is v5 and still bindings are here? Bindings are always separate >>> patches. Use subject prefixes matching the subsystem (git log --oneline >>> -- ...). >>> >>> And this was split, wasn't it? What happened here?!? >> >> Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history. >> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ > > Jerome was asking you to send 2 patchsets, one with : > - bindings in separate patches > - drivers in separate patches > and a second with DT changes. > > Then when the bindings + clocks patches are merged, a pull request of the bindings > can be done to me so I can merge it with DT. > >> >>> >>> >>>> MAINTAINERS | 1 + >>>> drivers/clk/meson/Kconfig | 13 + >>>> drivers/clk/meson/Makefile | 1 + >>>> drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ >>>> drivers/clk/meson/s4-pll.h | 88 ++ >>>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>>> 7 files changed, 1059 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>> create mode 100644 drivers/clk/meson/s4-pll.c >>>> create mode 100644 drivers/clk/meson/s4-pll.h >>>> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>> >>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>> new file mode 100644 >>>> index 000000000000..fd517e8ef14f >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>> @@ -0,0 +1,51 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: Amlogic Meson S serials PLL Clock Controller >>>> + >>>> +maintainers: >>>> + - Neil Armstrong <narmstrong@baylibre.com> >>>> + - Jerome Brunet <jbrunet@baylibre.com> >>>> + - Yu Tu <yu.hu@amlogic.com> >>>> + >>> One blank line. >> >> I will delete this, on next version patch. >> >>> >>>> + >>>> +properties: >>>> + compatible: >>>> + const: amlogic,s4-pll-clkc >>>> + >>>> + reg: >>>> + maxItems: 1 >>>> + >>>> + clocks: >>>> + maxItems: 1 >>>> + >>>> + clock-names: >>>> + items: >>>> + - const: xtal >>>> + >>>> + "#clock-cells": >>>> + const: 1 >>>> + >>>> +required: >>>> + - compatible >>>> + - reg >>>> + - clocks >>>> + - clock-names >>>> + - "#clock-cells" >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + clkc_pll: clock-controller@fe008000 { >>>> + compatible = "amlogic,s4-pll-clkc"; >>>> + reg = <0xfe008000 0x1e8>; >>>> + clocks = <&xtal>; >>>> + clock-names = "xtal"; >>>> + #clock-cells = <1>; >>>> + }; >>> >>> >>>> +#endif /* __MESON_S4_PLL_H__ */ >>>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>> new file mode 100644 >>>> index 000000000000..345f87023886 >>>> --- /dev/null >>>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>> >>> This belongs to bindings patch, not driver. >>> >>>> @@ -0,0 +1,30 @@ >>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>> +/* >>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>>> + * Author: Yu Tu <yu.tu@amlogic.com> >>>> + */ >>>> + >>>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>> + >>>> +/* >>>> + * CLKID index values >>>> + */ >>>> + >>>> +#define CLKID_FIXED_PLL 1 >>>> +#define CLKID_FCLK_DIV2 3 >>> >>> Indexes start from 0 and are incremented by 1. Not by 2. >>> >>> NAK. >> >> I remember Jerome discussing this with you.You can look at this submission history. >> https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ > > Historically we did that by only exposing part of the numbers, controlling which > clocks were part of the bindings. > > But it seems this doesn't make sens anymore, maybe it would be time to put all the > clock ids in the bindings for this new SoC and break with the previous strategy. So the outcome of the previous discussion was somewhere later in that thread: > It is just a choice to not expose some IDs. > It is not tied to the implementation at all. > I think we actually follow the rules and the idea behind it. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-23 13:53 ` Krzysztof Kozlowski @ 2022-11-25 9:23 ` Jerome Brunet 2022-11-28 7:39 ` Yu Tu 0 siblings, 1 reply; 34+ messages in thread From: Jerome Brunet @ 2022-11-25 9:23 UTC (permalink / raw) To: Krzysztof Kozlowski, neil.armstrong, Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On Wed 23 Nov 2022 at 14:53, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 23/11/2022 14:23, Neil Armstrong wrote: >> Hi, >> >> On 23/11/2022 12:16, Yu Tu wrote: >>> Hi Krzysztof, >>> Thank you for your reply. >>> >>> On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >>>> [ EXTERNAL EMAIL ] >>>> >>>> On 23/11/2022 03:13, Yu Tu wrote: >>>>> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >>>>> >>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>> --- >>>>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >>>> >>>> This is v5 and still bindings are here? Bindings are always separate >>>> patches. Use subject prefixes matching the subsystem (git log --oneline >>>> -- ...). >>>> >>>> And this was split, wasn't it? What happened here?!? >>> >>> Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history. >>> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ >> >> Jerome was asking you to send 2 patchsets, one with : >> - bindings in separate patches >> - drivers in separate patches >> and a second with DT changes. Indeed, this is what was asked. It is aligned with Krzysztof's request. >> >> Then when the bindings + clocks patches are merged, a pull request of the bindings >> can be done to me so I can merge it with DT. >> >>> >>>> >>>> >>>>> MAINTAINERS | 1 + >>>>> drivers/clk/meson/Kconfig | 13 + >>>>> drivers/clk/meson/Makefile | 1 + >>>>> drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ >>>>> drivers/clk/meson/s4-pll.h | 88 ++ >>>>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>>>> 7 files changed, 1059 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>> create mode 100644 drivers/clk/meson/s4-pll.c >>>>> create mode 100644 drivers/clk/meson/s4-pll.h >>>>> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>> new file mode 100644 >>>>> index 000000000000..fd517e8ef14f >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>> @@ -0,0 +1,51 @@ >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>> +%YAML 1.2 >>>>> +--- >>>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>> + >>>>> +title: Amlogic Meson S serials PLL Clock Controller >>>>> + >>>>> +maintainers: >>>>> + - Neil Armstrong <narmstrong@baylibre.com> >>>>> + - Jerome Brunet <jbrunet@baylibre.com> >>>>> + - Yu Tu <yu.hu@amlogic.com> >>>>> + >>>> One blank line. >>> >>> I will delete this, on next version patch. >>> >>>> >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + const: amlogic,s4-pll-clkc >>>>> + >>>>> + reg: >>>>> + maxItems: 1 >>>>> + >>>>> + clocks: >>>>> + maxItems: 1 >>>>> + >>>>> + clock-names: >>>>> + items: >>>>> + - const: xtal >>>>> + >>>>> + "#clock-cells": >>>>> + const: 1 >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - reg >>>>> + - clocks >>>>> + - clock-names >>>>> + - "#clock-cells" >>>>> + >>>>> +additionalProperties: false >>>>> + >>>>> +examples: >>>>> + - | >>>>> + clkc_pll: clock-controller@fe008000 { >>>>> + compatible = "amlogic,s4-pll-clkc"; >>>>> + reg = <0xfe008000 0x1e8>; >>>>> + clocks = <&xtal>; >>>>> + clock-names = "xtal"; >>>>> + #clock-cells = <1>; >>>>> + }; >>>> >>>> >>>>> +#endif /* __MESON_S4_PLL_H__ */ >>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>> new file mode 100644 >>>>> index 000000000000..345f87023886 >>>>> --- /dev/null >>>>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>> >>>> This belongs to bindings patch, not driver. >>>> >>>>> @@ -0,0 +1,30 @@ >>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>>> +/* >>>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>>>> + * Author: Yu Tu <yu.tu@amlogic.com> >>>>> + */ >>>>> + >>>>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>> + >>>>> +/* >>>>> + * CLKID index values >>>>> + */ >>>>> + >>>>> +#define CLKID_FIXED_PLL 1 >>>>> +#define CLKID_FCLK_DIV2 3 >>>> >>>> Indexes start from 0 and are incremented by 1. Not by 2. >>>> >>>> NAK. >>> >>> I remember Jerome discussing this with you.You can look at this submission history. >>> https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ >> >> Historically we did that by only exposing part of the numbers, controlling which >> clocks were part of the bindings. >> >> But it seems this doesn't make sens anymore, maybe it would be time to put all the >> clock ids in the bindings for this new SoC and break with the previous strategy. Krzysztof and I agreed there is nothing wrong with the current approach, I believe. It does not prevent someone from using an un-exposed clock, sure, or exposing it in the future if necessary. However, I think it clearly shows that an un-exposed element is not expected to be used by an external consumers. It should be enough to trigger a discussion if this expectation is wrong. > > So the outcome of the previous discussion was somewhere later in that > thread: > >> It is just a choice to not expose some IDs. >> It is not tied to the implementation at all. >> I think we actually follow the rules and the idea behind it. > > > Best regards, > Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-25 9:23 ` Jerome Brunet @ 2022-11-28 7:39 ` Yu Tu 2022-11-28 12:33 ` Jerome Brunet 0 siblings, 1 reply; 34+ messages in thread From: Yu Tu @ 2022-11-28 7:39 UTC (permalink / raw) To: Jerome Brunet, Krzysztof Kozlowski, neil.armstrong, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang Hi Jerome, Thank you for your reply. On 2022/11/25 17:23, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > > On Wed 23 Nov 2022 at 14:53, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 23/11/2022 14:23, Neil Armstrong wrote: >>> Hi, >>> >>> On 23/11/2022 12:16, Yu Tu wrote: >>>> Hi Krzysztof, >>>> Thank you for your reply. >>>> >>>> On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >>>>> [ EXTERNAL EMAIL ] >>>>> >>>>> On 23/11/2022 03:13, Yu Tu wrote: >>>>>> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >>>>>> >>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>>> --- >>>>>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >>>>> >>>>> This is v5 and still bindings are here? Bindings are always separate >>>>> patches. Use subject prefixes matching the subsystem (git log --oneline >>>>> -- ...). >>>>> >>>>> And this was split, wasn't it? What happened here?!? >>>> >>>> Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history. >>>> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ >>> >>> Jerome was asking you to send 2 patchsets, one with : >>> - bindings in separate patches >>> - drivers in separate patches >>> and a second with DT changes. > > Indeed, this is what was asked. It is aligned with Krzysztof's request. According to your discussion, I still should send patches in the previous way in series. But I'm going to change it like you suggested. I don't know, am I getting it right? > >>> >>> Then when the bindings + clocks patches are merged, a pull request of the bindings >>> can be done to me so I can merge it with DT. >>> >>>> >>>>> >>>>> >>>>>> MAINTAINERS | 1 + >>>>>> drivers/clk/meson/Kconfig | 13 + >>>>>> drivers/clk/meson/Makefile | 1 + >>>>>> drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ >>>>>> drivers/clk/meson/s4-pll.h | 88 ++ >>>>>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>>>>> 7 files changed, 1059 insertions(+) >>>>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>> create mode 100644 drivers/clk/meson/s4-pll.c >>>>>> create mode 100644 drivers/clk/meson/s4-pll.h >>>>>> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>> new file mode 100644 >>>>>> index 000000000000..fd517e8ef14f >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>> @@ -0,0 +1,51 @@ >>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>>> +%YAML 1.2 >>>>>> +--- >>>>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>> + >>>>>> +title: Amlogic Meson S serials PLL Clock Controller >>>>>> + >>>>>> +maintainers: >>>>>> + - Neil Armstrong <narmstrong@baylibre.com> >>>>>> + - Jerome Brunet <jbrunet@baylibre.com> >>>>>> + - Yu Tu <yu.hu@amlogic.com> >>>>>> + >>>>> One blank line. >>>> >>>> I will delete this, on next version patch. >>>> >>>>> >>>>>> + >>>>>> +properties: >>>>>> + compatible: >>>>>> + const: amlogic,s4-pll-clkc >>>>>> + >>>>>> + reg: >>>>>> + maxItems: 1 >>>>>> + >>>>>> + clocks: >>>>>> + maxItems: 1 >>>>>> + >>>>>> + clock-names: >>>>>> + items: >>>>>> + - const: xtal >>>>>> + >>>>>> + "#clock-cells": >>>>>> + const: 1 >>>>>> + >>>>>> +required: >>>>>> + - compatible >>>>>> + - reg >>>>>> + - clocks >>>>>> + - clock-names >>>>>> + - "#clock-cells" >>>>>> + >>>>>> +additionalProperties: false >>>>>> + >>>>>> +examples: >>>>>> + - | >>>>>> + clkc_pll: clock-controller@fe008000 { >>>>>> + compatible = "amlogic,s4-pll-clkc"; >>>>>> + reg = <0xfe008000 0x1e8>; >>>>>> + clocks = <&xtal>; >>>>>> + clock-names = "xtal"; >>>>>> + #clock-cells = <1>; >>>>>> + }; >>>>> >>>>> >>>>>> +#endif /* __MESON_S4_PLL_H__ */ >>>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>> new file mode 100644 >>>>>> index 000000000000..345f87023886 >>>>>> --- /dev/null >>>>>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>> >>>>> This belongs to bindings patch, not driver. >>>>> >>>>>> @@ -0,0 +1,30 @@ >>>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>>>> +/* >>>>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>>>>> + * Author: Yu Tu <yu.tu@amlogic.com> >>>>>> + */ >>>>>> + >>>>>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>> + >>>>>> +/* >>>>>> + * CLKID index values >>>>>> + */ >>>>>> + >>>>>> +#define CLKID_FIXED_PLL 1 >>>>>> +#define CLKID_FCLK_DIV2 3 >>>>> >>>>> Indexes start from 0 and are incremented by 1. Not by 2. >>>>> >>>>> NAK. >>>> >>>> I remember Jerome discussing this with you.You can look at this submission history. >>>> https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ >>> >>> Historically we did that by only exposing part of the numbers, controlling which >>> clocks were part of the bindings. >>> >>> But it seems this doesn't make sens anymore, maybe it would be time to put all the >>> clock ids in the bindings for this new SoC and break with the previous strategy. > > Krzysztof and I agreed there is nothing wrong with the current > approach, I believe. > > It does not prevent someone from using an un-exposed clock, sure, or > exposing it in the future if necessary. > > However, I think it clearly shows that an un-exposed element is not > expected to be used by an external consumers. It should be enough to > trigger a discussion if this expectation is wrong. > >> >> So the outcome of the previous discussion was somewhere later in that >> thread: >> >>> It is just a choice to not expose some IDs. >>> It is not tied to the implementation at all. >>> I think we actually follow the rules and the idea behind it. >> >> >> Best regards, >> Krzysztof > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-28 7:39 ` Yu Tu @ 2022-11-28 12:33 ` Jerome Brunet 2022-11-28 13:30 ` Yu Tu 0 siblings, 1 reply; 34+ messages in thread From: Jerome Brunet @ 2022-11-28 12:33 UTC (permalink / raw) To: Yu Tu, Krzysztof Kozlowski, neil.armstrong, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On Mon 28 Nov 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote: > Hi Jerome, > Thank you for your reply. > > On 2022/11/25 17:23, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> On Wed 23 Nov 2022 at 14:53, Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.org> wrote: >> >>> On 23/11/2022 14:23, Neil Armstrong wrote: >>>> Hi, >>>> >>>> On 23/11/2022 12:16, Yu Tu wrote: >>>>> Hi Krzysztof, >>>>> Thank you for your reply. >>>>> >>>>> On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >>>>>> [ EXTERNAL EMAIL ] >>>>>> >>>>>> On 23/11/2022 03:13, Yu Tu wrote: >>>>>>> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >>>>>>> >>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>>>> --- >>>>>>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >>>>>> >>>>>> This is v5 and still bindings are here? Bindings are always separate >>>>>> patches. Use subject prefixes matching the subsystem (git log --oneline >>>>>> -- ...). >>>>>> >>>>>> And this was split, wasn't it? What happened here?!? >>>>> >>>>> Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history. >>>>> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ >>>> >>>> Jerome was asking you to send 2 patchsets, one with : >>>> - bindings in separate patches >>>> - drivers in separate patches >>>> and a second with DT changes. >> Indeed, this is what was asked. It is aligned with Krzysztof's request. > > According to your discussion, I still should send patches in the previous > way in series. But I'm going to change it like you suggested. > I don't know, am I getting it right? 3 people tried to explain this already and we all told you the same thing. * 1 patchset per maintainer: clk and dt * bindings must be dedicated patches - never mixed with driver code. I strongly suggest that you take some time to (re)read: * https://docs.kernel.org/process/submitting-patches.html * https://docs.kernel.org/devicetree/bindings/submitting-patches.html If still unclear, please take some time to look at the kernel mailing list archive and see how others have done the same things. Thx. > >> >>>> >>>> Then when the bindings + clocks patches are merged, a pull request of the bindings >>>> can be done to me so I can merge it with DT. >>>> >>>>> >>>>>> >>>>>> >>>>>>> MAINTAINERS | 1 + >>>>>>> drivers/clk/meson/Kconfig | 13 + >>>>>>> drivers/clk/meson/Makefile | 1 + >>>>>>> drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ >>>>>>> drivers/clk/meson/s4-pll.h | 88 ++ >>>>>>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>>>>>> 7 files changed, 1059 insertions(+) >>>>>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>> create mode 100644 drivers/clk/meson/s4-pll.c >>>>>>> create mode 100644 drivers/clk/meson/s4-pll.h >>>>>>> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>> new file mode 100644 >>>>>>> index 000000000000..fd517e8ef14f >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>> @@ -0,0 +1,51 @@ >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>>>> +%YAML 1.2 >>>>>>> +--- >>>>>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>> + >>>>>>> +title: Amlogic Meson S serials PLL Clock Controller >>>>>>> + >>>>>>> +maintainers: >>>>>>> + - Neil Armstrong <narmstrong@baylibre.com> >>>>>>> + - Jerome Brunet <jbrunet@baylibre.com> >>>>>>> + - Yu Tu <yu.hu@amlogic.com> >>>>>>> + >>>>>> One blank line. >>>>> >>>>> I will delete this, on next version patch. >>>>> >>>>>> >>>>>>> + >>>>>>> +properties: >>>>>>> + compatible: >>>>>>> + const: amlogic,s4-pll-clkc >>>>>>> + >>>>>>> + reg: >>>>>>> + maxItems: 1 >>>>>>> + >>>>>>> + clocks: >>>>>>> + maxItems: 1 >>>>>>> + >>>>>>> + clock-names: >>>>>>> + items: >>>>>>> + - const: xtal >>>>>>> + >>>>>>> + "#clock-cells": >>>>>>> + const: 1 >>>>>>> + >>>>>>> +required: >>>>>>> + - compatible >>>>>>> + - reg >>>>>>> + - clocks >>>>>>> + - clock-names >>>>>>> + - "#clock-cells" >>>>>>> + >>>>>>> +additionalProperties: false >>>>>>> + >>>>>>> +examples: >>>>>>> + - | >>>>>>> + clkc_pll: clock-controller@fe008000 { >>>>>>> + compatible = "amlogic,s4-pll-clkc"; >>>>>>> + reg = <0xfe008000 0x1e8>; >>>>>>> + clocks = <&xtal>; >>>>>>> + clock-names = "xtal"; >>>>>>> + #clock-cells = <1>; >>>>>>> + }; >>>>>> >>>>>> >>>>>>> +#endif /* __MESON_S4_PLL_H__ */ >>>>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>> new file mode 100644 >>>>>>> index 000000000000..345f87023886 >>>>>>> --- /dev/null >>>>>>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>> >>>>>> This belongs to bindings patch, not driver. >>>>>> >>>>>>> @@ -0,0 +1,30 @@ >>>>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>>>>> +/* >>>>>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>>>>>> + * Author: Yu Tu <yu.tu@amlogic.com> >>>>>>> + */ >>>>>>> + >>>>>>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>>> + >>>>>>> +/* >>>>>>> + * CLKID index values >>>>>>> + */ >>>>>>> + >>>>>>> +#define CLKID_FIXED_PLL 1 >>>>>>> +#define CLKID_FCLK_DIV2 3 >>>>>> >>>>>> Indexes start from 0 and are incremented by 1. Not by 2. >>>>>> >>>>>> NAK. >>>>> >>>>> I remember Jerome discussing this with you.You can look at this submission history. >>>>> https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ >>>> >>>> Historically we did that by only exposing part of the numbers, controlling which >>>> clocks were part of the bindings. >>>> >>>> But it seems this doesn't make sens anymore, maybe it would be time to put all the >>>> clock ids in the bindings for this new SoC and break with the previous strategy. >> Krzysztof and I agreed there is nothing wrong with the current >> approach, I believe. >> It does not prevent someone from using an un-exposed clock, sure, or >> exposing it in the future if necessary. >> However, I think it clearly shows that an un-exposed element is not >> expected to be used by an external consumers. It should be enough to >> trigger a discussion if this expectation is wrong. >> >>> >>> So the outcome of the previous discussion was somewhere later in that >>> thread: >>> >>>> It is just a choice to not expose some IDs. >>>> It is not tied to the implementation at all. >>>> I think we actually follow the rules and the idea behind it. >>> >>> >>> Best regards, >>> Krzysztof >> . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-28 12:33 ` Jerome Brunet @ 2022-11-28 13:30 ` Yu Tu 2022-12-01 8:36 ` neil.armstrong 0 siblings, 1 reply; 34+ messages in thread From: Yu Tu @ 2022-11-28 13:30 UTC (permalink / raw) To: Jerome Brunet, Krzysztof Kozlowski, neil.armstrong, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang Hi Jerome , On 2022/11/28 20:33, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > > On Mon 28 Nov 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote: > >> Hi Jerome, >> Thank you for your reply. >> >> On 2022/11/25 17:23, Jerome Brunet wrote: >>> [ EXTERNAL EMAIL ] >>> On Wed 23 Nov 2022 at 14:53, Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 23/11/2022 14:23, Neil Armstrong wrote: >>>>> Hi, >>>>> >>>>> On 23/11/2022 12:16, Yu Tu wrote: >>>>>> Hi Krzysztof, >>>>>> Thank you for your reply. >>>>>> >>>>>> On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >>>>>>> [ EXTERNAL EMAIL ] >>>>>>> >>>>>>> On 23/11/2022 03:13, Yu Tu wrote: >>>>>>>> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >>>>>>>> >>>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>>>>> --- >>>>>>>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >>>>>>> >>>>>>> This is v5 and still bindings are here? Bindings are always separate >>>>>>> patches. Use subject prefixes matching the subsystem (git log --oneline >>>>>>> -- ...). >>>>>>> >>>>>>> And this was split, wasn't it? What happened here?!? >>>>>> >>>>>> Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history. >>>>>> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ >>>>> >>>>> Jerome was asking you to send 2 patchsets, one with : >>>>> - bindings in separate patches >>>>> - drivers in separate patches >>>>> and a second with DT changes. >>> Indeed, this is what was asked. It is aligned with Krzysztof's request. >> >> According to your discussion, I still should send patches in the previous >> way in series. But I'm going to change it like you suggested. >> I don't know, am I getting it right? > > 3 people tried to explain this already and we all told you the same thing. > > * 1 patchset per maintainer: clk and dt > * bindings must be dedicated patches - never mixed with driver code. > > I strongly suggest that you take some time to (re)read: > * https://docs.kernel.org/process/submitting-patches.html > * https://docs.kernel.org/devicetree/bindings/submitting-patches.html > > If still unclear, please take some time to look at the kernel mailing > list archive and see how others have done the same things. > > Thx. I'll change it as you suggest.But I still don't understand what you suggested in V3. I remember discussing it with you at V3. https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ ">>>> Also it would be nice to split this in two series. >>>> Bindings and drivers in one, arm64 dt in the other. These changes goes >>>> in through different trees. >>> At present, Bindings, DTS and drivers are three series. Do you mean to put >>> Bindings and drivers together? If so, checkpatch.pl will report a warning. >> Yes because patches are not in yet so there is a good reason to ignore >> the warning. Warning will never show up on the actual tree if the >> patches are correctly ordered. > > I think Binding, DTS and drivers use three series and you said two series > is not a big problem. Three series are recommended for checkpatch.pl, I > think it should be easy for that to separate and merge。 No - There is only 2 series. 1 for the bindings and clock drivers and one for the DT once things are in" > >> >>> >>>>> >>>>> Then when the bindings + clocks patches are merged, a pull request of the bindings >>>>> can be done to me so I can merge it with DT. >>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> MAINTAINERS | 1 + >>>>>>>> drivers/clk/meson/Kconfig | 13 + >>>>>>>> drivers/clk/meson/Makefile | 1 + >>>>>>>> drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ >>>>>>>> drivers/clk/meson/s4-pll.h | 88 ++ >>>>>>>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>>>>>>> 7 files changed, 1059 insertions(+) >>>>>>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>> create mode 100644 drivers/clk/meson/s4-pll.c >>>>>>>> create mode 100644 drivers/clk/meson/s4-pll.h >>>>>>>> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>> new file mode 100644 >>>>>>>> index 000000000000..fd517e8ef14f >>>>>>>> --- /dev/null >>>>>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>> @@ -0,0 +1,51 @@ >>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>>>>> +%YAML 1.2 >>>>>>>> +--- >>>>>>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>>> + >>>>>>>> +title: Amlogic Meson S serials PLL Clock Controller >>>>>>>> + >>>>>>>> +maintainers: >>>>>>>> + - Neil Armstrong <narmstrong@baylibre.com> >>>>>>>> + - Jerome Brunet <jbrunet@baylibre.com> >>>>>>>> + - Yu Tu <yu.hu@amlogic.com> >>>>>>>> + >>>>>>> One blank line. >>>>>> >>>>>> I will delete this, on next version patch. >>>>>> >>>>>>> >>>>>>>> + >>>>>>>> +properties: >>>>>>>> + compatible: >>>>>>>> + const: amlogic,s4-pll-clkc >>>>>>>> + >>>>>>>> + reg: >>>>>>>> + maxItems: 1 >>>>>>>> + >>>>>>>> + clocks: >>>>>>>> + maxItems: 1 >>>>>>>> + >>>>>>>> + clock-names: >>>>>>>> + items: >>>>>>>> + - const: xtal >>>>>>>> + >>>>>>>> + "#clock-cells": >>>>>>>> + const: 1 >>>>>>>> + >>>>>>>> +required: >>>>>>>> + - compatible >>>>>>>> + - reg >>>>>>>> + - clocks >>>>>>>> + - clock-names >>>>>>>> + - "#clock-cells" >>>>>>>> + >>>>>>>> +additionalProperties: false >>>>>>>> + >>>>>>>> +examples: >>>>>>>> + - | >>>>>>>> + clkc_pll: clock-controller@fe008000 { >>>>>>>> + compatible = "amlogic,s4-pll-clkc"; >>>>>>>> + reg = <0xfe008000 0x1e8>; >>>>>>>> + clocks = <&xtal>; >>>>>>>> + clock-names = "xtal"; >>>>>>>> + #clock-cells = <1>; >>>>>>>> + }; >>>>>>> >>>>>>> >>>>>>>> +#endif /* __MESON_S4_PLL_H__ */ >>>>>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>> new file mode 100644 >>>>>>>> index 000000000000..345f87023886 >>>>>>>> --- /dev/null >>>>>>>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>> >>>>>>> This belongs to bindings patch, not driver. >>>>>>> >>>>>>>> @@ -0,0 +1,30 @@ >>>>>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>>>>>> +/* >>>>>>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>>>>>>> + * Author: Yu Tu <yu.tu@amlogic.com> >>>>>>>> + */ >>>>>>>> + >>>>>>>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>>>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>>>> + >>>>>>>> +/* >>>>>>>> + * CLKID index values >>>>>>>> + */ >>>>>>>> + >>>>>>>> +#define CLKID_FIXED_PLL 1 >>>>>>>> +#define CLKID_FCLK_DIV2 3 >>>>>>> >>>>>>> Indexes start from 0 and are incremented by 1. Not by 2. >>>>>>> >>>>>>> NAK. >>>>>> >>>>>> I remember Jerome discussing this with you.You can look at this submission history. >>>>>> https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ >>>>> >>>>> Historically we did that by only exposing part of the numbers, controlling which >>>>> clocks were part of the bindings. >>>>> >>>>> But it seems this doesn't make sens anymore, maybe it would be time to put all the >>>>> clock ids in the bindings for this new SoC and break with the previous strategy. >>> Krzysztof and I agreed there is nothing wrong with the current >>> approach, I believe. >>> It does not prevent someone from using an un-exposed clock, sure, or >>> exposing it in the future if necessary. >>> However, I think it clearly shows that an un-exposed element is not >>> expected to be used by an external consumers. It should be enough to >>> trigger a discussion if this expectation is wrong. >>> >>>> >>>> So the outcome of the previous discussion was somewhere later in that >>>> thread: >>>> >>>>> It is just a choice to not expose some IDs. >>>>> It is not tied to the implementation at all. >>>>> I think we actually follow the rules and the idea behind it. >>>> >>>> >>>> Best regards, >>>> Krzysztof >>> . > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-28 13:30 ` Yu Tu @ 2022-12-01 8:36 ` neil.armstrong 2022-12-01 11:33 ` Yu Tu 0 siblings, 1 reply; 34+ messages in thread From: neil.armstrong @ 2022-12-01 8:36 UTC (permalink / raw) To: Yu Tu, Jerome Brunet, Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 28/11/2022 14:30, Yu Tu wrote: > Hi Jerome , > > On 2022/11/28 20:33, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> >> >> On Mon 28 Nov 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote: >> >>> Hi Jerome, >>> Thank you for your reply. >>> >>> On 2022/11/25 17:23, Jerome Brunet wrote: >>>> [ EXTERNAL EMAIL ] >>>> On Wed 23 Nov 2022 at 14:53, Krzysztof Kozlowski >>>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>>> On 23/11/2022 14:23, Neil Armstrong wrote: >>>>>> Hi, >>>>>> >>>>>> On 23/11/2022 12:16, Yu Tu wrote: >>>>>>> Hi Krzysztof, >>>>>>> Thank you for your reply. >>>>>>> >>>>>>> On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >>>>>>>> [ EXTERNAL EMAIL ] >>>>>>>> >>>>>>>> On 23/11/2022 03:13, Yu Tu wrote: >>>>>>>>> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >>>>>>>>> >>>>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>>>>>> --- >>>>>>>>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >>>>>>>> >>>>>>>> This is v5 and still bindings are here? Bindings are always separate >>>>>>>> patches. Use subject prefixes matching the subsystem (git log --oneline >>>>>>>> -- ...). >>>>>>>> >>>>>>>> And this was split, wasn't it? What happened here?!? >>>>>>> >>>>>>> Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history. >>>>>>> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ >>>>>> >>>>>> Jerome was asking you to send 2 patchsets, one with : >>>>>> - bindings in separate patches >>>>>> - drivers in separate patches >>>>>> and a second with DT changes. >>>> Indeed, this is what was asked. It is aligned with Krzysztof's request. >>> >>> According to your discussion, I still should send patches in the previous >>> way in series. But I'm going to change it like you suggested. >>> I don't know, am I getting it right? >> >> 3 people tried to explain this already and we all told you the same thing. >> >> * 1 patchset per maintainer: clk and dt >> * bindings must be dedicated patches - never mixed with driver code. >> >> I strongly suggest that you take some time to (re)read: >> * https://docs.kernel.org/process/submitting-patches.html >> * https://docs.kernel.org/devicetree/bindings/submitting-patches.html >> >> If still unclear, please take some time to look at the kernel mailing >> list archive and see how others have done the same things. >> >> Thx. > > I'll change it as you suggest.But I still don't understand what you suggested in V3. > > I remember discussing it with you at V3. > https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ > > ">>>> Also it would be nice to split this in two series. > >>>> Bindings and drivers in one, arm64 dt in the other. These changes goes > >>>> in through different trees. > >>> At present, Bindings, DTS and drivers are three series. Do you mean to put > >>> Bindings and drivers together? If so, checkpatch.pl will report a warning. > >> Yes because patches are not in yet so there is a good reason to ignore > >> the warning. Warning will never show up on the actual tree if the > >> patches are correctly ordered. > > > > I think Binding, DTS and drivers use three series and you said two series > > is not a big problem. Three series are recommended for checkpatch.pl, I > > think it should be easy for that to separate and merge。 > > No - There is only 2 series. 1 for the bindings and clock drivers and > one for the DT once things are in" Please send the following emails: * First patchset [PATCH V6 0/3] clk: meson: Add S4 SoC PLL and Peripheral clock controller [PATCH v6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & peripheral clock controller [PATCH v6 2/3] clk: meson: add support for Amlogic S4 SoC PLL [PATCH v6 3/3] clk: meson: add support for Amlogic S4 SoC peripheral clock controller 1) will contain only .yaml and dt-bindings include 2) will only have drivers/clk/meson changes 3) will only have drivers/clk/meson changes * Second patchset: [PATCH v1 0/2] arm64: dts: meson: Add S4 SoC PLL and Peripheral clock nodes [PATCH v1 1/2] arm64: dts: meson: add S4 Soc PLL clock controller node [PATCH v1 2/2] arm64: dts: meson: add S4 Soc Peripheral clock controller node 1) is the patch 3 of v5 patchset 2) is the patch 4 of v5 patchset And in the second cover letter, explain those patches comes from the previous V5 patchset and add a link to the V6 "drivers + bindings" patchset as a dependency. Neil > >> >>> >>>> >>>>>> >>>>>> Then when the bindings + clocks patches are merged, a pull request of the bindings >>>>>> can be done to me so I can merge it with DT. >>>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> MAINTAINERS | 1 + >>>>>>>>> drivers/clk/meson/Kconfig | 13 + >>>>>>>>> drivers/clk/meson/Makefile | 1 + >>>>>>>>> drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ >>>>>>>>> drivers/clk/meson/s4-pll.h | 88 ++ >>>>>>>>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>>>>>>>> 7 files changed, 1059 insertions(+) >>>>>>>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>>> create mode 100644 drivers/clk/meson/s4-pll.c >>>>>>>>> create mode 100644 drivers/clk/meson/s4-pll.h >>>>>>>>> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>>> >>>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>>> new file mode 100644 >>>>>>>>> index 000000000000..fd517e8ef14f >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>>> @@ -0,0 +1,51 @@ >>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>>>>>> +%YAML 1.2 >>>>>>>>> +--- >>>>>>>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>>>> + >>>>>>>>> +title: Amlogic Meson S serials PLL Clock Controller >>>>>>>>> + >>>>>>>>> +maintainers: >>>>>>>>> + - Neil Armstrong <narmstrong@baylibre.com> >>>>>>>>> + - Jerome Brunet <jbrunet@baylibre.com> >>>>>>>>> + - Yu Tu <yu.hu@amlogic.com> >>>>>>>>> + >>>>>>>> One blank line. >>>>>>> >>>>>>> I will delete this, on next version patch. >>>>>>> >>>>>>>> >>>>>>>>> + >>>>>>>>> +properties: >>>>>>>>> + compatible: >>>>>>>>> + const: amlogic,s4-pll-clkc >>>>>>>>> + >>>>>>>>> + reg: >>>>>>>>> + maxItems: 1 >>>>>>>>> + >>>>>>>>> + clocks: >>>>>>>>> + maxItems: 1 >>>>>>>>> + >>>>>>>>> + clock-names: >>>>>>>>> + items: >>>>>>>>> + - const: xtal >>>>>>>>> + >>>>>>>>> + "#clock-cells": >>>>>>>>> + const: 1 >>>>>>>>> + >>>>>>>>> +required: >>>>>>>>> + - compatible >>>>>>>>> + - reg >>>>>>>>> + - clocks >>>>>>>>> + - clock-names >>>>>>>>> + - "#clock-cells" >>>>>>>>> + >>>>>>>>> +additionalProperties: false >>>>>>>>> + >>>>>>>>> +examples: >>>>>>>>> + - | >>>>>>>>> + clkc_pll: clock-controller@fe008000 { >>>>>>>>> + compatible = "amlogic,s4-pll-clkc"; >>>>>>>>> + reg = <0xfe008000 0x1e8>; >>>>>>>>> + clocks = <&xtal>; >>>>>>>>> + clock-names = "xtal"; >>>>>>>>> + #clock-cells = <1>; >>>>>>>>> + }; >>>>>>>> >>>>>>>> >>>>>>>>> +#endif /* __MESON_S4_PLL_H__ */ >>>>>>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>>> new file mode 100644 >>>>>>>>> index 000000000000..345f87023886 >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>> >>>>>>>> This belongs to bindings patch, not driver. >>>>>>>> >>>>>>>>> @@ -0,0 +1,30 @@ >>>>>>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>>>>>>> +/* >>>>>>>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>>>>>>>> + * Author: Yu Tu <yu.tu@amlogic.com> >>>>>>>>> + */ >>>>>>>>> + >>>>>>>>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>>>>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>>>>> + >>>>>>>>> +/* >>>>>>>>> + * CLKID index values >>>>>>>>> + */ >>>>>>>>> + >>>>>>>>> +#define CLKID_FIXED_PLL 1 >>>>>>>>> +#define CLKID_FCLK_DIV2 3 >>>>>>>> >>>>>>>> Indexes start from 0 and are incremented by 1. Not by 2. >>>>>>>> >>>>>>>> NAK. >>>>>>> >>>>>>> I remember Jerome discussing this with you.You can look at this submission history. >>>>>>> https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ >>>>>> >>>>>> Historically we did that by only exposing part of the numbers, controlling which >>>>>> clocks were part of the bindings. >>>>>> >>>>>> But it seems this doesn't make sens anymore, maybe it would be time to put all the >>>>>> clock ids in the bindings for this new SoC and break with the previous strategy. >>>> Krzysztof and I agreed there is nothing wrong with the current >>>> approach, I believe. >>>> It does not prevent someone from using an un-exposed clock, sure, or >>>> exposing it in the future if necessary. >>>> However, I think it clearly shows that an un-exposed element is not >>>> expected to be used by an external consumers. It should be enough to >>>> trigger a discussion if this expectation is wrong. >>>> >>>>> >>>>> So the outcome of the previous discussion was somewhere later in that >>>>> thread: >>>>> >>>>>> It is just a choice to not expose some IDs. >>>>>> It is not tied to the implementation at all. >>>>>> I think we actually follow the rules and the idea behind it. >>>>> >>>>> >>>>> Best regards, >>>>> Krzysztof >>>> . >> >> . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-12-01 8:36 ` neil.armstrong @ 2022-12-01 11:33 ` Yu Tu 0 siblings, 0 replies; 34+ messages in thread From: Yu Tu @ 2022-12-01 11:33 UTC (permalink / raw) To: neil.armstrong, Jerome Brunet, Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 2022/12/1 16:36, neil.armstrong@linaro.org wrote: > [ EXTERNAL EMAIL ] > > On 28/11/2022 14:30, Yu Tu wrote: >> Hi Jerome , >> >> On 2022/11/28 20:33, Jerome Brunet wrote: >>> [ EXTERNAL EMAIL ] >>> >>> >>> On Mon 28 Nov 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote: >>> >>>> Hi Jerome, >>>> Thank you for your reply. >>>> >>>> On 2022/11/25 17:23, Jerome Brunet wrote: >>>>> [ EXTERNAL EMAIL ] >>>>> On Wed 23 Nov 2022 at 14:53, Krzysztof Kozlowski >>>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>> >>>>>> On 23/11/2022 14:23, Neil Armstrong wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 23/11/2022 12:16, Yu Tu wrote: >>>>>>>> Hi Krzysztof, >>>>>>>> Thank you for your reply. >>>>>>>> >>>>>>>> On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >>>>>>>>> [ EXTERNAL EMAIL ] >>>>>>>>> >>>>>>>>> On 23/11/2022 03:13, Yu Tu wrote: >>>>>>>>>> Add the S4 PLL clock controller found and bindings in the s4 >>>>>>>>>> SoC family. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>>>>>>> --- >>>>>>>>>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >>>>>>>>> >>>>>>>>> This is v5 and still bindings are here? Bindings are always >>>>>>>>> separate >>>>>>>>> patches. Use subject prefixes matching the subsystem (git log >>>>>>>>> --oneline >>>>>>>>> -- ...). >>>>>>>>> >>>>>>>>> And this was split, wasn't it? What happened here?!? >>>>>>>> >>>>>>>> Put bindings and clock driver patch together from Jerome. Maybe >>>>>>>> you can read this chat history. >>>>>>>> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ >>>>>>> >>>>>>> Jerome was asking you to send 2 patchsets, one with : >>>>>>> - bindings in separate patches >>>>>>> - drivers in separate patches >>>>>>> and a second with DT changes. >>>>> Indeed, this is what was asked. It is aligned with Krzysztof's >>>>> request. >>>> >>>> According to your discussion, I still should send patches in the >>>> previous >>>> way in series. But I'm going to change it like you suggested. >>>> I don't know, am I getting it right? >>> >>> 3 people tried to explain this already and we all told you the same >>> thing. >>> >>> * 1 patchset per maintainer: clk and dt >>> * bindings must be dedicated patches - never mixed with driver code. >>> >>> I strongly suggest that you take some time to (re)read: >>> * https://docs.kernel.org/process/submitting-patches.html >>> * https://docs.kernel.org/devicetree/bindings/submitting-patches.html >>> >>> If still unclear, please take some time to look at the kernel mailing >>> list archive and see how others have done the same things. >>> >>> Thx. >> >> I'll change it as you suggest.But I still don't understand what you >> suggested in V3. >> >> I remember discussing it with you at V3. >> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ >> >> ">>>> Also it would be nice to split this in two series. >> >>>> Bindings and drivers in one, arm64 dt in the other. These >> changes goes >> >>>> in through different trees. >> >>> At present, Bindings, DTS and drivers are three series. Do you >> mean to put >> >>> Bindings and drivers together? If so, checkpatch.pl will report a >> warning. >> >> Yes because patches are not in yet so there is a good reason to >> ignore >> >> the warning. Warning will never show up on the actual tree if the >> >> patches are correctly ordered. >> > >> > I think Binding, DTS and drivers use three series and you said two >> series >> > is not a big problem. Three series are recommended for >> checkpatch.pl, I >> > think it should be easy for that to separate and merge。 >> >> No - There is only 2 series. 1 for the bindings and clock drivers and >> one for the DT once things are in" > > Please send the following emails: > > * First patchset > > [PATCH V6 0/3] clk: meson: Add S4 SoC PLL and Peripheral clock controller > [PATCH v6 1/3] dt-bindings: clock: document Amlogic S4 SoC PLL & > peripheral clock controller > [PATCH v6 2/3] clk: meson: add support for Amlogic S4 SoC PLL > [PATCH v6 3/3] clk: meson: add support for Amlogic S4 SoC > peripheral clock controller > > 1) will contain only .yaml and dt-bindings include > 2) will only have drivers/clk/meson changes > 3) will only have drivers/clk/meson changes > > * Second patchset: > > [PATCH v1 0/2] arm64: dts: meson: Add S4 SoC PLL and Peripheral clock nodes > [PATCH v1 1/2] arm64: dts: meson: add S4 Soc PLL clock controller node > [PATCH v1 2/2] arm64: dts: meson: add S4 Soc Peripheral clock > controller node > > 1) is the patch 3 of v5 patchset > 2) is the patch 4 of v5 patchset > > And in the second cover letter, explain those patches comes from the > previous V5 patchset > and add a link to the V6 "drivers + bindings" patchset as a dependency. > > Neil Hi Neil, Thank you very much for your detailed explanation. >> >>> >>>> >>>>> >>>>>>> >>>>>>> Then when the bindings + clocks patches are merged, a pull >>>>>>> request of the bindings >>>>>>> can be done to me so I can merge it with DT. >>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> MAINTAINERS | 1 + >>>>>>>>>> drivers/clk/meson/Kconfig | 13 + >>>>>>>>>> drivers/clk/meson/Makefile | 1 + >>>>>>>>>> drivers/clk/meson/s4-pll.c | 875 >>>>>>>>>> ++++++++++++++++++ >>>>>>>>>> drivers/clk/meson/s4-pll.h | 88 ++ >>>>>>>>>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>>>>>>>>> 7 files changed, 1059 insertions(+) >>>>>>>>>> create mode 100644 >>>>>>>>>> Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>>>> create mode 100644 drivers/clk/meson/s4-pll.c >>>>>>>>>> create mode 100644 drivers/clk/meson/s4-pll.h >>>>>>>>>> create mode 100644 >>>>>>>>>> include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>>>> >>>>>>>>>> diff --git >>>>>>>>>> a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>>>> new file mode 100644 >>>>>>>>>> index 000000000000..fd517e8ef14f >>>>>>>>>> --- /dev/null >>>>>>>>>> +++ >>>>>>>>>> b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>>>>>>> @@ -0,0 +1,51 @@ >>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>>>>>>> +%YAML 1.2 >>>>>>>>>> +--- >>>>>>>>>> +$id: >>>>>>>>>> http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>>>>> + >>>>>>>>>> +title: Amlogic Meson S serials PLL Clock Controller >>>>>>>>>> + >>>>>>>>>> +maintainers: >>>>>>>>>> + - Neil Armstrong <narmstrong@baylibre.com> >>>>>>>>>> + - Jerome Brunet <jbrunet@baylibre.com> >>>>>>>>>> + - Yu Tu <yu.hu@amlogic.com> >>>>>>>>>> + >>>>>>>>> One blank line. >>>>>>>> >>>>>>>> I will delete this, on next version patch. >>>>>>>> >>>>>>>>> >>>>>>>>>> + >>>>>>>>>> +properties: >>>>>>>>>> + compatible: >>>>>>>>>> + const: amlogic,s4-pll-clkc >>>>>>>>>> + >>>>>>>>>> + reg: >>>>>>>>>> + maxItems: 1 >>>>>>>>>> + >>>>>>>>>> + clocks: >>>>>>>>>> + maxItems: 1 >>>>>>>>>> + >>>>>>>>>> + clock-names: >>>>>>>>>> + items: >>>>>>>>>> + - const: xtal >>>>>>>>>> + >>>>>>>>>> + "#clock-cells": >>>>>>>>>> + const: 1 >>>>>>>>>> + >>>>>>>>>> +required: >>>>>>>>>> + - compatible >>>>>>>>>> + - reg >>>>>>>>>> + - clocks >>>>>>>>>> + - clock-names >>>>>>>>>> + - "#clock-cells" >>>>>>>>>> + >>>>>>>>>> +additionalProperties: false >>>>>>>>>> + >>>>>>>>>> +examples: >>>>>>>>>> + - | >>>>>>>>>> + clkc_pll: clock-controller@fe008000 { >>>>>>>>>> + compatible = "amlogic,s4-pll-clkc"; >>>>>>>>>> + reg = <0xfe008000 0x1e8>; >>>>>>>>>> + clocks = <&xtal>; >>>>>>>>>> + clock-names = "xtal"; >>>>>>>>>> + #clock-cells = <1>; >>>>>>>>>> + }; >>>>>>>>> >>>>>>>>> >>>>>>>>>> +#endif /* __MESON_S4_PLL_H__ */ >>>>>>>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>>>> b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>>>> new file mode 100644 >>>>>>>>>> index 000000000000..345f87023886 >>>>>>>>>> --- /dev/null >>>>>>>>>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>>>>>> >>>>>>>>> This belongs to bindings patch, not driver. >>>>>>>>> >>>>>>>>>> @@ -0,0 +1,30 @@ >>>>>>>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>>>>>>>> +/* >>>>>>>>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>>>>>>>>> + * Author: Yu Tu <yu.tu@amlogic.com> >>>>>>>>>> + */ >>>>>>>>>> + >>>>>>>>>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>>>>>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>>>>>>> + >>>>>>>>>> +/* >>>>>>>>>> + * CLKID index values >>>>>>>>>> + */ >>>>>>>>>> + >>>>>>>>>> +#define CLKID_FIXED_PLL 1 >>>>>>>>>> +#define CLKID_FCLK_DIV2 3 >>>>>>>>> >>>>>>>>> Indexes start from 0 and are incremented by 1. Not by 2. >>>>>>>>> >>>>>>>>> NAK. >>>>>>>> >>>>>>>> I remember Jerome discussing this with you.You can look at this >>>>>>>> submission history. >>>>>>>> https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ >>>>>>> >>>>>>> Historically we did that by only exposing part of the numbers, >>>>>>> controlling which >>>>>>> clocks were part of the bindings. >>>>>>> >>>>>>> But it seems this doesn't make sens anymore, maybe it would be >>>>>>> time to put all the >>>>>>> clock ids in the bindings for this new SoC and break with the >>>>>>> previous strategy. >>>>> Krzysztof and I agreed there is nothing wrong with the current >>>>> approach, I believe. >>>>> It does not prevent someone from using an un-exposed clock, sure, or >>>>> exposing it in the future if necessary. >>>>> However, I think it clearly shows that an un-exposed element is not >>>>> expected to be used by an external consumers. It should be enough to >>>>> trigger a discussion if this expectation is wrong. >>>>> >>>>>> >>>>>> So the outcome of the previous discussion was somewhere later in that >>>>>> thread: >>>>>> >>>>>>> It is just a choice to not expose some IDs. >>>>>>> It is not tied to the implementation at all. >>>>>>> I think we actually follow the rules and the idea behind it. >>>>>> >>>>>> >>>>>> Best regards, >>>>>> Krzysztof >>>>> . >>> >>> . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-23 13:23 ` Neil Armstrong 2022-11-23 13:53 ` Krzysztof Kozlowski @ 2022-11-23 13:54 ` Yu Tu 2022-11-23 13:57 ` neil.armstrong 1 sibling, 1 reply; 34+ messages in thread From: Yu Tu @ 2022-11-23 13:54 UTC (permalink / raw) To: neil.armstrong, Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang Hi Neil, On 2022/11/23 21:23, Neil Armstrong wrote: > [ EXTERNAL EMAIL ] > > Hi, > > On 23/11/2022 12:16, Yu Tu wrote: >> Hi Krzysztof, >> Thank you for your reply. >> >> On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On 23/11/2022 03:13, Yu Tu wrote: >>>> Add the S4 PLL clock controller found and bindings in the s4 SoC >>>> family. >>>> >>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>> --- >>>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >>> >>> This is v5 and still bindings are here? Bindings are always separate >>> patches. Use subject prefixes matching the subsystem (git log --oneline >>> -- ...). >>> >>> And this was split, wasn't it? What happened here?!? >> >> Put bindings and clock driver patch together from Jerome. Maybe you >> can read this chat history. >> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ > > Jerome was asking you to send 2 patchsets, one with : > - bindings in separate patches > - drivers in separate patches > and a second with DT changes. > > Then when the bindings + clocks patches are merged, a pull request of > the bindings > can be done to me so I can merge it with DT. > I may have misunderstood Jerome's advice.So should I follow the V4 patch format and change as Jerome suggested from V3? >> >>> >>> >>>> MAINTAINERS | 1 + >>>> drivers/clk/meson/Kconfig | 13 + >>>> drivers/clk/meson/Makefile | 1 + >>>> drivers/clk/meson/s4-pll.c | 875 >>>> ++++++++++++++++++ >>>> drivers/clk/meson/s4-pll.h | 88 ++ >>>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>>> 7 files changed, 1059 insertions(+) >>>> create mode 100644 >>>> Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>> create mode 100644 drivers/clk/meson/s4-pll.c >>>> create mode 100644 drivers/clk/meson/s4-pll.h >>>> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>> b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>> new file mode 100644 >>>> index 000000000000..fd517e8ef14f >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>> @@ -0,0 +1,51 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: Amlogic Meson S serials PLL Clock Controller >>>> + >>>> +maintainers: >>>> + - Neil Armstrong <narmstrong@baylibre.com> >>>> + - Jerome Brunet <jbrunet@baylibre.com> >>>> + - Yu Tu <yu.hu@amlogic.com> >>>> + >>> One blank line. >> >> I will delete this, on next version patch. >> >>> >>>> + >>>> +properties: >>>> + compatible: >>>> + const: amlogic,s4-pll-clkc >>>> + >>>> + reg: >>>> + maxItems: 1 >>>> + >>>> + clocks: >>>> + maxItems: 1 >>>> + >>>> + clock-names: >>>> + items: >>>> + - const: xtal >>>> + >>>> + "#clock-cells": >>>> + const: 1 >>>> + >>>> +required: >>>> + - compatible >>>> + - reg >>>> + - clocks >>>> + - clock-names >>>> + - "#clock-cells" >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + clkc_pll: clock-controller@fe008000 { >>>> + compatible = "amlogic,s4-pll-clkc"; >>>> + reg = <0xfe008000 0x1e8>; >>>> + clocks = <&xtal>; >>>> + clock-names = "xtal"; >>>> + #clock-cells = <1>; >>>> + }; >>> >>> >>>> +#endif /* __MESON_S4_PLL_H__ */ >>>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>> b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>> new file mode 100644 >>>> index 000000000000..345f87023886 >>>> --- /dev/null >>>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>> >>> This belongs to bindings patch, not driver. >>> >>>> @@ -0,0 +1,30 @@ >>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>> +/* >>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>>> + * Author: Yu Tu <yu.tu@amlogic.com> >>>> + */ >>>> + >>>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>> + >>>> +/* >>>> + * CLKID index values >>>> + */ >>>> + >>>> +#define CLKID_FIXED_PLL 1 >>>> +#define CLKID_FCLK_DIV2 3 >>> >>> Indexes start from 0 and are incremented by 1. Not by 2. >>> >>> NAK. >> >> I remember Jerome discussing this with you.You can look at this >> submission history. >> https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ >> > > Historically we did that by only exposing part of the numbers, > controlling which > clocks were part of the bindings. > > But it seems this doesn't make sens anymore, maybe it would be time to > put all the > clock ids in the bindings for this new SoC and break with the previous > strategy. That's OK with me. But I don't know if Jerome thinks it's ok? > > Neil > >> >>> >>> Best regards, >>> Krzysztof >>> >>> . > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings 2022-11-23 13:54 ` Yu Tu @ 2022-11-23 13:57 ` neil.armstrong 0 siblings, 0 replies; 34+ messages in thread From: neil.armstrong @ 2022-11-23 13:57 UTC (permalink / raw) To: Yu Tu, Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 14:54, Yu Tu wrote: > Hi Neil, > > On 2022/11/23 21:23, Neil Armstrong wrote: >> [ EXTERNAL EMAIL ] >> >> Hi, >> >> On 23/11/2022 12:16, Yu Tu wrote: >>> Hi Krzysztof, >>> Thank you for your reply. >>> >>> On 2022/11/23 18:08, Krzysztof Kozlowski wrote: >>>> [ EXTERNAL EMAIL ] >>>> >>>> On 23/11/2022 03:13, Yu Tu wrote: >>>>> Add the S4 PLL clock controller found and bindings in the s4 SoC family. >>>>> >>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>> --- >>>>> .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + >>>> >>>> This is v5 and still bindings are here? Bindings are always separate >>>> patches. Use subject prefixes matching the subsystem (git log --oneline >>>> -- ...). >>>> >>>> And this was split, wasn't it? What happened here?!? >>> >>> Put bindings and clock driver patch together from Jerome. Maybe you can read this chat history. >>> https://lore.kernel.or/all/1jy1v6z14n.fsf@starbuckisacylon.baylibre.com/ >> >> Jerome was asking you to send 2 patchsets, one with : >> - bindings in separate patches >> - drivers in separate patches >> and a second with DT changes. >> >> Then when the bindings + clocks patches are merged, a pull request of the bindings >> can be done to me so I can merge it with DT. >> > > I may have misunderstood Jerome's advice.So should I follow the V4 patch format and change as Jerome suggested from V3? Let's wait for his input on this to see if a v4 is needed now or later. > >>> >>>> >>>> >>>>> MAINTAINERS | 1 + >>>>> drivers/clk/meson/Kconfig | 13 + >>>>> drivers/clk/meson/Makefile | 1 + >>>>> drivers/clk/meson/s4-pll.c | 875 ++++++++++++++++++ >>>>> drivers/clk/meson/s4-pll.h | 88 ++ >>>>> .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + >>>>> 7 files changed, 1059 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>> create mode 100644 drivers/clk/meson/s4-pll.c >>>>> create mode 100644 drivers/clk/meson/s4-pll.h >>>>> create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>> new file mode 100644 >>>>> index 000000000000..fd517e8ef14f >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml >>>>> @@ -0,0 +1,51 @@ >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>> +%YAML 1.2 >>>>> +--- >>>>> +$id: http://devicetree.org/schemas/clock/amlogic,s4-pll-clkc.yaml# >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>> + >>>>> +title: Amlogic Meson S serials PLL Clock Controller >>>>> + >>>>> +maintainers: >>>>> + - Neil Armstrong <narmstrong@baylibre.com> >>>>> + - Jerome Brunet <jbrunet@baylibre.com> >>>>> + - Yu Tu <yu.hu@amlogic.com> >>>>> + >>>> One blank line. >>> >>> I will delete this, on next version patch. >>> >>>> >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + const: amlogic,s4-pll-clkc >>>>> + >>>>> + reg: >>>>> + maxItems: 1 >>>>> + >>>>> + clocks: >>>>> + maxItems: 1 >>>>> + >>>>> + clock-names: >>>>> + items: >>>>> + - const: xtal >>>>> + >>>>> + "#clock-cells": >>>>> + const: 1 >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - reg >>>>> + - clocks >>>>> + - clock-names >>>>> + - "#clock-cells" >>>>> + >>>>> +additionalProperties: false >>>>> + >>>>> +examples: >>>>> + - | >>>>> + clkc_pll: clock-controller@fe008000 { >>>>> + compatible = "amlogic,s4-pll-clkc"; >>>>> + reg = <0xfe008000 0x1e8>; >>>>> + clocks = <&xtal>; >>>>> + clock-names = "xtal"; >>>>> + #clock-cells = <1>; >>>>> + }; >>>> >>>> >>>>> +#endif /* __MESON_S4_PLL_H__ */ >>>>> diff --git a/include/dt-bindings/clock/amlogic,s4-pll-clkc.h b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>>> new file mode 100644 >>>>> index 000000000000..345f87023886 >>>>> --- /dev/null >>>>> +++ b/include/dt-bindings/clock/amlogic,s4-pll-clkc.h >>>> >>>> This belongs to bindings patch, not driver. >>>> >>>>> @@ -0,0 +1,30 @@ >>>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>>> +/* >>>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved. >>>>> + * Author: Yu Tu <yu.tu@amlogic.com> >>>>> + */ >>>>> + >>>>> +#ifndef _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>> +#define _DT_BINDINGS_CLOCK_AMLOGIC_S4_PLL_CLKC_H >>>>> + >>>>> +/* >>>>> + * CLKID index values >>>>> + */ >>>>> + >>>>> +#define CLKID_FIXED_PLL 1 >>>>> +#define CLKID_FCLK_DIV2 3 >>>> >>>> Indexes start from 0 and are incremented by 1. Not by 2. >>>> >>>> NAK. >>> >>> I remember Jerome discussing this with you.You can look at this submission history. >>> https://lore.kernel.org/all/c088e01c-0714-82be-8347-6140daf56640@linaro.org/ >> >> Historically we did that by only exposing part of the numbers, controlling which >> clocks were part of the bindings. >> >> But it seems this doesn't make sens anymore, maybe it would be time to put all the >> clock ids in the bindings for this new SoC and break with the previous strategy. > > That's OK with me. But I don't know if Jerome thinks it's ok? Let's wait for his input on that aswell. Neil > >> >> Neil >> >>> >>>> >>>> Best regards, >>>> Krzysztof >>>> >>>> . >> >> . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH V5 2/4] arm64: dts: meson: add S4 Soc PLL clock controller in DT 2022-11-23 2:13 [PATCH V5 0/4] Add S4 SoC PLL and Peripheral clock controller Yu Tu 2022-11-23 2:13 ` [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings Yu Tu @ 2022-11-23 2:13 ` Yu Tu 2022-11-23 2:13 ` [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral " Yu Tu [not found] ` <20221123021346.18136-4-yu.tu@amlogic.com> 3 siblings, 0 replies; 34+ messages in thread From: Yu Tu @ 2022-11-23 2:13 UTC (permalink / raw) To: linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang, Yu Tu Added information about the S4 SOC PLL Clock controller in DT. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi index ad50cba42d19..bd9c2ef83314 100644 --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi @@ -92,6 +92,14 @@ apb4: apb4@fe000000 { #size-cells = <2>; ranges = <0x0 0x0 0x0 0xfe000000 0x0 0x480000>; + clkc_pll: clock-controller@8000 { + compatible = "amlogic,s4-pll-clkc"; + reg = <0x0 0x8000 0x0 0x1e8>; + clocks = <&xtal>; + clock-names = "xtal"; + #clock-cells = <1>; + }; + periphs_pinctrl: pinctrl@4000 { compatible = "amlogic,meson-s4-periphs-pinctrl"; #address-cells = <2>; -- 2.33.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 2:13 [PATCH V5 0/4] Add S4 SoC PLL and Peripheral clock controller Yu Tu 2022-11-23 2:13 ` [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings Yu Tu 2022-11-23 2:13 ` [PATCH V5 2/4] arm64: dts: meson: add S4 Soc PLL clock controller in DT Yu Tu @ 2022-11-23 2:13 ` Yu Tu 2022-11-23 10:10 ` Krzysztof Kozlowski [not found] ` <20221123021346.18136-4-yu.tu@amlogic.com> 3 siblings, 1 reply; 34+ messages in thread From: Yu Tu @ 2022-11-23 2:13 UTC (permalink / raw) To: linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang, Yu Tu Added information about the S4 SOC Peripheral Clock controller in DT. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi index bd9c2ef83314..e7fab6e400be 100644 --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi @@ -6,6 +6,8 @@ #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> / { cpus { @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { #clock-cells = <1>; }; + clkc_periphs: clock-controller { + compatible = "amlogic,s4-peripherals-clkc"; + reg = <0x0 0x0 0x0 0x49c>; + clocks = <&clkc_pll CLKID_FCLK_DIV2>, + <&clkc_pll CLKID_FCLK_DIV2P5>, + <&clkc_pll CLKID_FCLK_DIV3>, + <&clkc_pll CLKID_FCLK_DIV4>, + <&clkc_pll CLKID_FCLK_DIV5>, + <&clkc_pll CLKID_FCLK_DIV7>, + <&clkc_pll CLKID_HIFI_PLL>, + <&clkc_pll CLKID_GP0_PLL>, + <&clkc_pll CLKID_MPLL0>, + <&clkc_pll CLKID_MPLL1>, + <&clkc_pll CLKID_MPLL2>, + <&clkc_pll CLKID_MPLL3>, + <&clkc_pll CLKID_HDMI_PLL>, + <&xtal>; + clock-names = "fclk_div2", "fclk_div2p5", "fclk_div3", + "fclk_div4", "fclk_div5", "fclk_div7", + "hifi_pll", "gp0_pll", "mpll0", "mpll1", + "mpll2", "mpll3", "hdmi_pll", "xtal"; + #clock-cells = <1>; + }; + periphs_pinctrl: pinctrl@4000 { compatible = "amlogic,meson-s4-periphs-pinctrl"; #address-cells = <2>; -- 2.33.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 2:13 ` [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral " Yu Tu @ 2022-11-23 10:10 ` Krzysztof Kozlowski 2022-11-23 11:27 ` Yu Tu 2022-11-23 13:27 ` Neil Armstrong 0 siblings, 2 replies; 34+ messages in thread From: Krzysztof Kozlowski @ 2022-11-23 10:10 UTC (permalink / raw) To: Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 03:13, Yu Tu wrote: > Added information about the S4 SOC Peripheral Clock controller in DT. > > Signed-off-by: Yu Tu <yu.tu@amlogic.com> > --- > arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi > index bd9c2ef83314..e7fab6e400be 100644 > --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi > +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi > @@ -6,6 +6,8 @@ > #include <dt-bindings/interrupt-controller/irq.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> > +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> > > / { > cpus { > @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { > #clock-cells = <1>; > }; > > + clkc_periphs: clock-controller { > + compatible = "amlogic,s4-peripherals-clkc"; > + reg = <0x0 0x0 0x0 0x49c>; This is broken... did you check for warnings? Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 10:10 ` Krzysztof Kozlowski @ 2022-11-23 11:27 ` Yu Tu 2022-11-23 13:02 ` Krzysztof Kozlowski 2022-11-23 13:27 ` Neil Armstrong 1 sibling, 1 reply; 34+ messages in thread From: Yu Tu @ 2022-11-23 11:27 UTC (permalink / raw) To: Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang Hi Krzysztof, On 2022/11/23 18:10, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 03:13, Yu Tu wrote: >> Added information about the S4 SOC Peripheral Clock controller in DT. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> --- >> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> index bd9c2ef83314..e7fab6e400be 100644 >> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> @@ -6,6 +6,8 @@ >> #include <dt-bindings/interrupt-controller/irq.h> >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> #include <dt-bindings/gpio/gpio.h> >> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >> >> / { >> cpus { >> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >> #clock-cells = <1>; >> }; >> >> + clkc_periphs: clock-controller { >> + compatible = "amlogic,s4-peripherals-clkc"; >> + reg = <0x0 0x0 0x0 0x49c>; > > This is broken... did you check for warnings? Yes, i do. You can have a look at the results of my test, as follows. total: 0 errors, 0 warnings, 0 checks, 38 lines checked ../patch_clk_v5_1122/0004-arm64-dts-meson-add-S4-Soc-Peripheral-clock-controll.patch has no obvious style problems and is ready for submission. > > > Best regards, > Krzysztof > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 11:27 ` Yu Tu @ 2022-11-23 13:02 ` Krzysztof Kozlowski 2022-11-23 13:23 ` Yu Tu 0 siblings, 1 reply; 34+ messages in thread From: Krzysztof Kozlowski @ 2022-11-23 13:02 UTC (permalink / raw) To: Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 12:27, Yu Tu wrote: > Hi Krzysztof, > > On 2022/11/23 18:10, Krzysztof Kozlowski wrote: >> [ EXTERNAL EMAIL ] >> >> On 23/11/2022 03:13, Yu Tu wrote: >>> Added information about the S4 SOC Peripheral Clock controller in DT. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> index bd9c2ef83314..e7fab6e400be 100644 >>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> @@ -6,6 +6,8 @@ >>> #include <dt-bindings/interrupt-controller/irq.h> >>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>> #include <dt-bindings/gpio/gpio.h> >>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>> >>> / { >>> cpus { >>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>> #clock-cells = <1>; >>> }; >>> >>> + clkc_periphs: clock-controller { >>> + compatible = "amlogic,s4-peripherals-clkc"; >>> + reg = <0x0 0x0 0x0 0x49c>; >> >> This is broken... did you check for warnings? > Yes, i do. > You can have a look at the results of my test, as follows. > > total: 0 errors, 0 warnings, 0 checks, 38 lines checked > > ../patch_clk_v5_1122/0004-arm64-dts-meson-add-S4-Soc-Peripheral-clock-controll.patch > has no obvious style problems and is ready for submission. > This is a checkpatch output. I am talking about DTS broken. dtc should warn you. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 13:02 ` Krzysztof Kozlowski @ 2022-11-23 13:23 ` Yu Tu 2022-11-23 14:12 ` Krzysztof Kozlowski 0 siblings, 1 reply; 34+ messages in thread From: Yu Tu @ 2022-11-23 13:23 UTC (permalink / raw) To: Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 2022/11/23 21:02, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 12:27, Yu Tu wrote: >> Hi Krzysztof, >> >> On 2022/11/23 18:10, Krzysztof Kozlowski wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On 23/11/2022 03:13, Yu Tu wrote: >>>> Added information about the S4 SOC Peripheral Clock controller in DT. >>>> >>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>> --- >>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >>>> 1 file changed, 26 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> index bd9c2ef83314..e7fab6e400be 100644 >>>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> @@ -6,6 +6,8 @@ >>>> #include <dt-bindings/interrupt-controller/irq.h> >>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>> #include <dt-bindings/gpio/gpio.h> >>>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>>> >>>> / { >>>> cpus { >>>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>>> #clock-cells = <1>; >>>> }; >>>> >>>> + clkc_periphs: clock-controller { >>>> + compatible = "amlogic,s4-peripherals-clkc"; >>>> + reg = <0x0 0x0 0x0 0x49c>; >>> >>> This is broken... did you check for warnings? >> Yes, i do. >> You can have a look at the results of my test, as follows. >> >> total: 0 errors, 0 warnings, 0 checks, 38 lines checked >> >> ../patch_clk_v5_1122/0004-arm64-dts-meson-add-S4-Soc-Peripheral-clock-controll.patch >> has no obvious style problems and is ready for submission. >> > > This is a checkpatch output. I am talking about DTS broken. dtc should > warn you. Do you mean I will have wraning in compiling? I actually compiled without warning. ccf$ make ARCH=arm64 dtbs -j12 DTC arch/arm64/boot/dts/amlogic/meson-s4-s805x2-aq222.dtb > > Best regards, > Krzysztof > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 13:23 ` Yu Tu @ 2022-11-23 14:12 ` Krzysztof Kozlowski 2022-11-23 14:23 ` Yu Tu 0 siblings, 1 reply; 34+ messages in thread From: Krzysztof Kozlowski @ 2022-11-23 14:12 UTC (permalink / raw) To: Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 14:23, Yu Tu wrote: >>> ../patch_clk_v5_1122/0004-arm64-dts-meson-add-S4-Soc-Peripheral-clock-controll.patch >>> has no obvious style problems and is ready for submission. >>> >> >> This is a checkpatch output. I am talking about DTS broken. dtc should >> warn you. > > Do you mean I will have wraning in compiling? > I actually compiled without warning. > ccf$ make ARCH=arm64 dtbs -j12 > DTC arch/arm64/boot/dts/amlogic/meson-s4-s805x2-aq222.dtb > Nope: ../arch/arm64/boot/dts/amlogic/meson-s4.dtsi:105.35-127.6: Warning (unit_address_vs_reg): /soc/apb4@fe000000/clock-controller: node has a reg or ranges property, but no unit name Test your patches better before using reviewers time for trivial compile-time fixes. The same rules for C code apply for DTS. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 14:12 ` Krzysztof Kozlowski @ 2022-11-23 14:23 ` Yu Tu 0 siblings, 0 replies; 34+ messages in thread From: Yu Tu @ 2022-11-23 14:23 UTC (permalink / raw) To: Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 2022/11/23 22:12, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 14:23, Yu Tu wrote: >>>> ../patch_clk_v5_1122/0004-arm64-dts-meson-add-S4-Soc-Peripheral-clock-controll.patch >>>> has no obvious style problems and is ready for submission. >>>> >>> >>> This is a checkpatch output. I am talking about DTS broken. dtc should >>> warn you. >> >> Do you mean I will have wraning in compiling? >> I actually compiled without warning. >> ccf$ make ARCH=arm64 dtbs -j12 >> DTC arch/arm64/boot/dts/amlogic/meson-s4-s805x2-aq222.dtb >> > > Nope: > > ../arch/arm64/boot/dts/amlogic/meson-s4.dtsi:105.35-127.6: Warning > (unit_address_vs_reg): /soc/apb4@fe000000/clock-controller: node has a > reg or ranges property, but no unit name > > Test your patches better before using reviewers time for trivial > compile-time fixes. The same rules for C code apply for DTS. > Like this: clkc_periphs: clock-controller@0 { compatible = "amlogic,s4-peripherals-clkc"; reg = <0x0 0x0 0x0 0x49c>; You mean it should be, right? > Best regards, > Krzysztof > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 10:10 ` Krzysztof Kozlowski 2022-11-23 11:27 ` Yu Tu @ 2022-11-23 13:27 ` Neil Armstrong 2022-11-23 13:38 ` Yu Tu 2022-11-23 14:13 ` Krzysztof Kozlowski 1 sibling, 2 replies; 34+ messages in thread From: Neil Armstrong @ 2022-11-23 13:27 UTC (permalink / raw) To: Krzysztof Kozlowski, Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 11:10, Krzysztof Kozlowski wrote: > On 23/11/2022 03:13, Yu Tu wrote: >> Added information about the S4 SOC Peripheral Clock controller in DT. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> --- >> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> index bd9c2ef83314..e7fab6e400be 100644 >> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> @@ -6,6 +6,8 @@ >> #include <dt-bindings/interrupt-controller/irq.h> >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> #include <dt-bindings/gpio/gpio.h> >> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >> >> / { >> cpus { >> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >> #clock-cells = <1>; >> }; >> >> + clkc_periphs: clock-controller { >> + compatible = "amlogic,s4-peripherals-clkc"; >> + reg = <0x0 0x0 0x0 0x49c>; > > This is broken... did you check for warnings? This is actually fine, the parent node has a ranges property: https://github.com/torvalds/linux/blob/eb7081409f94a9a8608593d0fb63a1aa3d6f95d8/arch/arm64/boot/dts/amlogic/meson-s4.dtsi#L93 Neil > > > Best regards, > Krzysztof > > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 13:27 ` Neil Armstrong @ 2022-11-23 13:38 ` Yu Tu 2022-11-23 14:13 ` Krzysztof Kozlowski 1 sibling, 0 replies; 34+ messages in thread From: Yu Tu @ 2022-11-23 13:38 UTC (permalink / raw) To: neil.armstrong, Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang Hi Neil, Thanks for your reply and explanation. On 2022/11/23 21:27, Neil Armstrong wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 11:10, Krzysztof Kozlowski wrote: >> On 23/11/2022 03:13, Yu Tu wrote: >>> Added information about the S4 SOC Peripheral Clock controller in DT. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> index bd9c2ef83314..e7fab6e400be 100644 >>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> @@ -6,6 +6,8 @@ >>> #include <dt-bindings/interrupt-controller/irq.h> >>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>> #include <dt-bindings/gpio/gpio.h> >>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>> / { >>> cpus { >>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>> #clock-cells = <1>; >>> }; >>> + clkc_periphs: clock-controller { >>> + compatible = "amlogic,s4-peripherals-clkc"; >>> + reg = <0x0 0x0 0x0 0x49c>; >> >> This is broken... did you check for warnings? > > This is actually fine, the parent node has a ranges property: > https://github.com/torvalds/linux/blob/eb7081409f94a9a8608593d0fb63a1aa3d6f95d8/arch/arm64/boot/dts/amlogic/meson-s4.dtsi#L93 > > > Neil > >> >> >> Best regards, >> Krzysztof >> >> >> _______________________________________________ >> linux-amlogic mailing list >> linux-amlogic@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-amlogic > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 13:27 ` Neil Armstrong 2022-11-23 13:38 ` Yu Tu @ 2022-11-23 14:13 ` Krzysztof Kozlowski 2022-11-23 14:21 ` neil.armstrong 1 sibling, 1 reply; 34+ messages in thread From: Krzysztof Kozlowski @ 2022-11-23 14:13 UTC (permalink / raw) To: neil.armstrong, Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 14:27, Neil Armstrong wrote: > On 23/11/2022 11:10, Krzysztof Kozlowski wrote: >> On 23/11/2022 03:13, Yu Tu wrote: >>> Added information about the S4 SOC Peripheral Clock controller in DT. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> index bd9c2ef83314..e7fab6e400be 100644 >>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> @@ -6,6 +6,8 @@ >>> #include <dt-bindings/interrupt-controller/irq.h> >>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>> #include <dt-bindings/gpio/gpio.h> >>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>> >>> / { >>> cpus { >>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>> #clock-cells = <1>; >>> }; >>> >>> + clkc_periphs: clock-controller { >>> + compatible = "amlogic,s4-peripherals-clkc"; >>> + reg = <0x0 0x0 0x0 0x49c>; >> >> This is broken... did you check for warnings? > > This is actually fine, the parent node has a ranges property: > https://github.com/torvalds/linux/blob/eb7081409f94a9a8608593d0fb63a1aa3d6f95d8/arch/arm64/boot/dts/amlogic/meson-s4.dtsi#L93 The parent ranges do not change here anything. You cannot have a reg without unit address and the tools report it. No need to use reviewers for this... Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 14:13 ` Krzysztof Kozlowski @ 2022-11-23 14:21 ` neil.armstrong 2022-11-23 14:27 ` Yu Tu 0 siblings, 1 reply; 34+ messages in thread From: neil.armstrong @ 2022-11-23 14:21 UTC (permalink / raw) To: Krzysztof Kozlowski, Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 15:13, Krzysztof Kozlowski wrote: > On 23/11/2022 14:27, Neil Armstrong wrote: >> On 23/11/2022 11:10, Krzysztof Kozlowski wrote: >>> On 23/11/2022 03:13, Yu Tu wrote: >>>> Added information about the S4 SOC Peripheral Clock controller in DT. >>>> >>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>> --- >>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >>>> 1 file changed, 26 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> index bd9c2ef83314..e7fab6e400be 100644 >>>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> @@ -6,6 +6,8 @@ >>>> #include <dt-bindings/interrupt-controller/irq.h> >>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>> #include <dt-bindings/gpio/gpio.h> >>>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>>> >>>> / { >>>> cpus { >>>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>>> #clock-cells = <1>; >>>> }; >>>> >>>> + clkc_periphs: clock-controller { >>>> + compatible = "amlogic,s4-peripherals-clkc"; >>>> + reg = <0x0 0x0 0x0 0x49c>; >>> >>> This is broken... did you check for warnings? >> >> This is actually fine, the parent node has a ranges property: >> https://github.com/torvalds/linux/blob/eb7081409f94a9a8608593d0fb63a1aa3d6f95d8/arch/arm64/boot/dts/amlogic/meson-s4.dtsi#L93 > > The parent ranges do not change here anything. You cannot have a reg > without unit address and the tools report it. No need to use reviewers > for this... Oh I see now, I simply looked at the reg property, not the unit address. Sorry indeed it's wrong and DTC should complain. Yu, you should probably update your DTC version. Neil > > Best regards, > Krzysztof > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT 2022-11-23 14:21 ` neil.armstrong @ 2022-11-23 14:27 ` Yu Tu 0 siblings, 0 replies; 34+ messages in thread From: Yu Tu @ 2022-11-23 14:27 UTC (permalink / raw) To: neil.armstrong, Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 2022/11/23 22:21, neil.armstrong@linaro.org wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 15:13, Krzysztof Kozlowski wrote: >> On 23/11/2022 14:27, Neil Armstrong wrote: >>> On 23/11/2022 11:10, Krzysztof Kozlowski wrote: >>>> On 23/11/2022 03:13, Yu Tu wrote: >>>>> Added information about the S4 SOC Peripheral Clock controller in DT. >>>>> >>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>> --- >>>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 >>>>> +++++++++++++++++++++++ >>>>> 1 file changed, 26 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>>> b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>>> index bd9c2ef83314..e7fab6e400be 100644 >>>>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>>> @@ -6,6 +6,8 @@ >>>>> #include <dt-bindings/interrupt-controller/irq.h> >>>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>> #include <dt-bindings/gpio/gpio.h> >>>>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>>>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>>>> / { >>>>> cpus { >>>>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>>>> #clock-cells = <1>; >>>>> }; >>>>> + clkc_periphs: clock-controller { >>>>> + compatible = "amlogic,s4-peripherals-clkc"; >>>>> + reg = <0x0 0x0 0x0 0x49c>; >>>> >>>> This is broken... did you check for warnings? >>> >>> This is actually fine, the parent node has a ranges property: >>> https://github.com/torvalds/linux/blob/eb7081409f94a9a8608593d0fb63a1aa3d6f95d8/arch/arm64/boot/dts/amlogic/meson-s4.dtsi#L93 >>> >> >> The parent ranges do not change here anything. You cannot have a reg >> without unit address and the tools report it. No need to use reviewers >> for this... > > Oh I see now, I simply looked at the reg property, not the unit address. > > Sorry indeed it's wrong and DTC should complain. > > Yu, you should probably update your DTC version. Okay. > > Neil > >> >> Best regards, >> Krzysztof >> > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <20221123021346.18136-4-yu.tu@amlogic.com>]
* Re: [PATCH V5 3/4] clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings [not found] ` <20221123021346.18136-4-yu.tu@amlogic.com> @ 2022-11-23 10:09 ` Krzysztof Kozlowski 2022-11-23 11:22 ` Yu Tu [not found] ` <1jbkov2vb9.fsf@starbuckisacylon.baylibre.com> 1 sibling, 1 reply; 34+ messages in thread From: Krzysztof Kozlowski @ 2022-11-23 10:09 UTC (permalink / raw) To: Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 03:13, Yu Tu wrote: > Add the peripherals clock controller found and bindings in the s4 SoC family. > > Signed-off-by: Yu Tu <yu.tu@amlogic.com> > --- > .../clock/amlogic,s4-peripherals-clkc.yaml | 105 + No, this is total mess now. Additionally, you received a lot of feedback but your changelog says only: "V3 -> V4: change format and clock flags." so you ignored entire feedback? That's not the way to work with patches. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 3/4] clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings 2022-11-23 10:09 ` [PATCH V5 3/4] clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings Krzysztof Kozlowski @ 2022-11-23 11:22 ` Yu Tu 2022-11-23 13:06 ` Krzysztof Kozlowski 0 siblings, 1 reply; 34+ messages in thread From: Yu Tu @ 2022-11-23 11:22 UTC (permalink / raw) To: Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 2022/11/23 18:09, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 03:13, Yu Tu wrote: >> Add the peripherals clock controller found and bindings in the s4 SoC family. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> --- >> .../clock/amlogic,s4-peripherals-clkc.yaml | 105 + > > No, this is total mess now. > > Additionally, you received a lot of feedback but your changelog says only: > "V3 -> V4: change format and clock flags." > so you ignored entire feedback? > > That's not the way to work with patches. Hi Krzysztof, You can check the previous email reply. Now I don't know who to follow your advice or Jerome's. I'm confused. Maybe you need to come to a conclusion. So I can change it in the next patch. > > Best regards, > Krzysztof > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 3/4] clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings 2022-11-23 11:22 ` Yu Tu @ 2022-11-23 13:06 ` Krzysztof Kozlowski 2022-11-23 14:08 ` Yu Tu 0 siblings, 1 reply; 34+ messages in thread From: Krzysztof Kozlowski @ 2022-11-23 13:06 UTC (permalink / raw) To: Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On 23/11/2022 12:22, Yu Tu wrote: > > > On 2022/11/23 18:09, Krzysztof Kozlowski wrote: >> [ EXTERNAL EMAIL ] >> >> On 23/11/2022 03:13, Yu Tu wrote: >>> Add the peripherals clock controller found and bindings in the s4 SoC family. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> .../clock/amlogic,s4-peripherals-clkc.yaml | 105 + >> >> No, this is total mess now. >> >> Additionally, you received a lot of feedback but your changelog says only: >> "V3 -> V4: change format and clock flags." >> so you ignored entire feedback? >> >> That's not the way to work with patches. > > Hi Krzysztof, > You can check the previous email reply. Now I don't know who to follow > your advice or Jerome's. I'm confused. Maybe you need to come to a > conclusion. So I can change it in the next patch. I don't understand your comment. You received a lot of things to change for your v3. You said here "change format and clock flagS", so all other feedbacks from me were ignored? They were not contradicting to Jerome's comments, so either you implement them and mention this in changelog, or you keep discussing. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 3/4] clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings 2022-11-23 13:06 ` Krzysztof Kozlowski @ 2022-11-23 14:08 ` Yu Tu 0 siblings, 0 replies; 34+ messages in thread From: Yu Tu @ 2022-11-23 14:08 UTC (permalink / raw) To: Krzysztof Kozlowski, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Jerome Brunet, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang Hi Krzysztof, On 2022/11/23 21:06, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 12:22, Yu Tu wrote: >> >> >> On 2022/11/23 18:09, Krzysztof Kozlowski wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On 23/11/2022 03:13, Yu Tu wrote: >>>> Add the peripherals clock controller found and bindings in the s4 SoC family. >>>> >>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>> --- >>>> .../clock/amlogic,s4-peripherals-clkc.yaml | 105 + >>> >>> No, this is total mess now. >>> >>> Additionally, you received a lot of feedback but your changelog says only: >>> "V3 -> V4: change format and clock flags." >>> so you ignored entire feedback? >>> >>> That's not the way to work with patches. >> >> Hi Krzysztof, >> You can check the previous email reply. Now I don't know who to follow >> your advice or Jerome's. I'm confused. Maybe you need to come to a >> conclusion. So I can change it in the next patch. > > I don't understand your comment. You received a lot of things to change > for your v3. You said here "change format and clock flagS", so all other > feedbacks from me were ignored? They were not contradicting to Jerome's > comments, so either you implement them and mention this in changelog, or > you keep discussing. > I'm sorry I didn't write that clearly. The reality is that V4 I have changed some of the suggestions from Jerome. But there was a part that didn't agree before I sent V4, which Jerome chose to skip. Let's continue with V3, and then prepare this V5. > Best regards, > Krzysztof > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <1jbkov2vb9.fsf@starbuckisacylon.baylibre.com>]
[parent not found: <81d9a794-2920-64f1-1d80-50653113624c@amlogic.com>]
* Re: [PATCH V5 3/4] clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings [not found] ` <81d9a794-2920-64f1-1d80-50653113624c@amlogic.com> @ 2022-11-28 12:23 ` Jerome Brunet 2022-11-28 14:02 ` Yu Tu 0 siblings, 1 reply; 34+ messages in thread From: Jerome Brunet @ 2022-11-28 12:23 UTC (permalink / raw) To: Yu Tu, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang On Mon 28 Nov 2022 at 16:08, Yu Tu <yu.tu@amlogic.com> wrote: >>> + >>> +/* >>> + * This RTC clock can be supplied by an external 32KHz crystal oscillator. >>> + * If it is used, it should be documented in using fw_name and documented in the >>> + * Bindings. Not currently in use on this board, so skip it. >>> + */ >>> +static u32 rtc_clk_sel[] = { 0, 1 }; >> No reason to do that > > I'm going to change it to static u32 rtc_clk_sel[] = { 0, 1, 2 };. > I don't know if that's okay with you? ... then there is no need to specify this table. > >> >>> +static const struct clk_parent_data rtc_clk_sel_parent_data[] = { >>> + { .hw = &s4_rtc_32k_by_oscin.hw }, >>> + { .hw = &s4_rtc_32k_by_oscin_div.hw }, >>> + { .fw_name = "ext_32k", } >>> +}; >>> + >>> +static struct clk_regmap s4_rtc_clk = { >>> + .data = &(struct clk_regmap_mux_data) { >>> + .offset = CLKCTRL_RTC_CTRL, >>> + .mask = 0x3, >>> + .shift = 0, >>> + .table = rtc_clk_sel, >>> + .flags = CLK_MUX_ROUND_CLOSEST, >>> + }, >>> + .hw.init = &(struct clk_init_data){ >>> + .name = "rtc_clk_sel", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = rtc_clk_sel_parent_data, >>> + .num_parents = 2, >>> + .flags = CLK_SET_RATE_PARENT, >>> + }, >>> +}; >>> + [...] >>> + >>> +/* Video Clocks */ >>> +static struct clk_regmap s4_vid_pll_div = { >>> + .data = &(struct meson_vid_pll_div_data){ >>> + .val = { >>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>> + .shift = 0, >>> + .width = 15, >>> + }, >>> + .sel = { >>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>> + .shift = 16, >>> + .width = 2, >>> + }, >>> + }, >>> + .hw.init = &(struct clk_init_data) { >>> + .name = "vid_pll_div", >>> + /* Same to g12a */ >>> + .ops = &meson_vid_pll_div_ro_ops, >> Please add an helpful explanation. >> 'Same to g12a' is not helpful. >> > > "Because the vid_pll_div clock is a clock that does not need to change the > divisor, ops only provides meson_vid_pll_div_ro_ops." > I wonder if this description is ok for you? I understand this divider will not change with RO ops. I'm interrested why it does not change and how it is expected to be setup. > >>> + .parent_data = (const struct clk_parent_data []) { >>> + { .fw_name = "hdmi_pll", } >>> + }, >>> + .num_parents = 1, >>> + .flags = CLK_SET_RATE_PARENT, >>> + }, >>> +}; [...] >>> + >>> +static struct clk_regmap s4_vclk_sel = { >>> + .data = &(struct clk_regmap_mux_data){ >>> + .offset = CLKCTRL_VID_CLK_CTRL, >>> + .mask = 0x7, >>> + .shift = 16, >>> + }, >>> + .hw.init = &(struct clk_init_data){ >>> + .name = "vclk_sel", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = s4_vclk_parent_data, >>> + .num_parents = ARRAY_SIZE(s4_vclk_parent_data), >>> + }, >> You are stopping rate propagation here. >> It deserves an explanation. Same goes below. > > "When the driver uses this clock, needs to specify the patent clock he > wants in the dts." > Is ok for you? Then you still don't understand the clock flag usage. Preserving the parent selection (CLK_SET_RATE_NO_REPARENT) and rate propagation (CLK_SET_RATE_PARENT) is not the same thing. As it stands, your comment is not aliged with what you do. > >> >>> +}; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH V5 3/4] clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings 2022-11-28 12:23 ` Jerome Brunet @ 2022-11-28 14:02 ` Yu Tu 0 siblings, 0 replies; 34+ messages in thread From: Yu Tu @ 2022-11-28 14:02 UTC (permalink / raw) To: Jerome Brunet, linux-clk, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree, Rob Herring, Neil Armstrong, Kevin Hilman, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Martin Blumenstingl Cc: kelvin.zhang Hi Jerome, On 2022/11/28 20:23, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > > On Mon 28 Nov 2022 at 16:08, Yu Tu <yu.tu@amlogic.com> wrote: > >>>> + >>>> +/* >>>> + * This RTC clock can be supplied by an external 32KHz crystal oscillator. >>>> + * If it is used, it should be documented in using fw_name and documented in the >>>> + * Bindings. Not currently in use on this board, so skip it. >>>> + */ >>>> +static u32 rtc_clk_sel[] = { 0, 1 }; >>> No reason to do that >> >> I'm going to change it to static u32 rtc_clk_sel[] = { 0, 1, 2 };. >> I don't know if that's okay with you? > > ... then there is no need to specify this table. > I got it.I'll change it as you suggest. > > >> >>> >>>> +static const struct clk_parent_data rtc_clk_sel_parent_data[] = { >>>> + { .hw = &s4_rtc_32k_by_oscin.hw }, >>>> + { .hw = &s4_rtc_32k_by_oscin_div.hw }, >>>> + { .fw_name = "ext_32k", } >>>> +}; >>>> + >>>> +static struct clk_regmap s4_rtc_clk = { >>>> + .data = &(struct clk_regmap_mux_data) { >>>> + .offset = CLKCTRL_RTC_CTRL, >>>> + .mask = 0x3, >>>> + .shift = 0, >>>> + .table = rtc_clk_sel, >>>> + .flags = CLK_MUX_ROUND_CLOSEST, >>>> + }, >>>> + .hw.init = &(struct clk_init_data){ >>>> + .name = "rtc_clk_sel", >>>> + .ops = &clk_regmap_mux_ops, >>>> + .parent_data = rtc_clk_sel_parent_data, >>>> + .num_parents = 2, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> + }, >>>> +}; >>>> + > > [...] > >>>> + >>>> +/* Video Clocks */ >>>> +static struct clk_regmap s4_vid_pll_div = { >>>> + .data = &(struct meson_vid_pll_div_data){ >>>> + .val = { >>>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>>> + .shift = 0, >>>> + .width = 15, >>>> + }, >>>> + .sel = { >>>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>>> + .shift = 16, >>>> + .width = 2, >>>> + }, >>>> + }, >>>> + .hw.init = &(struct clk_init_data) { >>>> + .name = "vid_pll_div", >>>> + /* Same to g12a */ >>>> + .ops = &meson_vid_pll_div_ro_ops, >>> Please add an helpful explanation. >>> 'Same to g12a' is not helpful. >>> >> >> "Because the vid_pll_div clock is a clock that does not need to change the >> divisor, ops only provides meson_vid_pll_div_ro_ops." >> I wonder if this description is ok for you? > > I understand this divider will not change with RO ops. > I'm interrested why it does not change and how it is expected to be setup. > Maybe you can be more specific, I don't understand, you're interested in that part of it specifically. I don't know if you have the document of chip. If not, I can provide it to you privately. You can ask specific questions in conjunction with your documentation and submission(The original submission came from you.). I can give you a specific answer or ask the chip designer to give you a reply.Do you think that's okay with you >> >>>> + .parent_data = (const struct clk_parent_data []) { >>>> + { .fw_name = "hdmi_pll", } >>>> + }, >>>> + .num_parents = 1, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> + }, >>>> +}; > > [...] > >>>> + >>>> +static struct clk_regmap s4_vclk_sel = { >>>> + .data = &(struct clk_regmap_mux_data){ >>>> + .offset = CLKCTRL_VID_CLK_CTRL, >>>> + .mask = 0x7, >>>> + .shift = 16, >>>> + }, >>>> + .hw.init = &(struct clk_init_data){ >>>> + .name = "vclk_sel", >>>> + .ops = &clk_regmap_mux_ops, >>>> + .parent_data = s4_vclk_parent_data, >>>> + .num_parents = ARRAY_SIZE(s4_vclk_parent_data), >>>> + }, >>> You are stopping rate propagation here. >>> It deserves an explanation. Same goes below. >> >> "When the driver uses this clock, needs to specify the patent clock he >> wants in the dts." >> Is ok for you? > > Then you still don't understand the clock flag usage. > > Preserving the parent selection (CLK_SET_RATE_NO_REPARENT) and rate > propagation (CLK_SET_RATE_PARENT) is not the same thing. > > As it stands, your comment is not aliged with what you do. > Thanks for the explanation of flag. My goal is to have the clock user describe themselves in DTS using the parent, or using the assigned-clocks and assigned-clock-parents settings in DTS. According to your explanation, some clocks like this should use CLK_SET_RATE_NO_REPARENT, right? >> >>> >>>> +}; > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2022-12-01 11:34 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-23 2:13 [PATCH V5 0/4] Add S4 SoC PLL and Peripheral clock controller Yu Tu
2022-11-23 2:13 ` [PATCH V5 1/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings Yu Tu
2022-11-23 10:08 ` Krzysztof Kozlowski
2022-11-23 11:16 ` Yu Tu
2022-11-23 13:05 ` Krzysztof Kozlowski
2022-11-23 13:23 ` Neil Armstrong
2022-11-23 13:53 ` Krzysztof Kozlowski
2022-11-25 9:23 ` Jerome Brunet
2022-11-28 7:39 ` Yu Tu
2022-11-28 12:33 ` Jerome Brunet
2022-11-28 13:30 ` Yu Tu
2022-12-01 8:36 ` neil.armstrong
2022-12-01 11:33 ` Yu Tu
2022-11-23 13:54 ` Yu Tu
2022-11-23 13:57 ` neil.armstrong
2022-11-23 2:13 ` [PATCH V5 2/4] arm64: dts: meson: add S4 Soc PLL clock controller in DT Yu Tu
2022-11-23 2:13 ` [PATCH V5 4/4] arm64: dts: meson: add S4 Soc Peripheral " Yu Tu
2022-11-23 10:10 ` Krzysztof Kozlowski
2022-11-23 11:27 ` Yu Tu
2022-11-23 13:02 ` Krzysztof Kozlowski
2022-11-23 13:23 ` Yu Tu
2022-11-23 14:12 ` Krzysztof Kozlowski
2022-11-23 14:23 ` Yu Tu
2022-11-23 13:27 ` Neil Armstrong
2022-11-23 13:38 ` Yu Tu
2022-11-23 14:13 ` Krzysztof Kozlowski
2022-11-23 14:21 ` neil.armstrong
2022-11-23 14:27 ` Yu Tu
[not found] ` <20221123021346.18136-4-yu.tu@amlogic.com>
2022-11-23 10:09 ` [PATCH V5 3/4] clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings Krzysztof Kozlowski
2022-11-23 11:22 ` Yu Tu
2022-11-23 13:06 ` Krzysztof Kozlowski
2022-11-23 14:08 ` Yu Tu
[not found] ` <1jbkov2vb9.fsf@starbuckisacylon.baylibre.com>
[not found] ` <81d9a794-2920-64f1-1d80-50653113624c@amlogic.com>
2022-11-28 12:23 ` Jerome Brunet
2022-11-28 14:02 ` Yu Tu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).