Let's start from the end: > No - Looks to me you just have two clock controllers you are trying force into one. > Again, this shows 2 devices. The one related to your 'map0' should request AUD2_CLKID_AUDIOTOP as input and enable it right away. Most of fishy workarounds that you commented is caused the fact the mmio of this clock controller is divided into two parts. Compare it with axg-audio driver, things that was part of contigous memory region (like pdm) here are moved to second region. Is this enough to make a guess that these are two devices? Concerning AUD2_CLKID_AUDIOTOP clock, as it turned out, it must be enabled before enabling of clocks from second region too. That is AUD2_CLKID_AUDIOTOP clock feeds both parts of this clock controller. On 3/15/24 12:20, Jerome Brunet wrote: > > On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> This controller provides clocks and reset functionality for audio >> peripherals on Amlogic A1 SoC family. >> >> The driver is almost identical to 'axg-audio', however it would be better >> to keep it separate due to following reasons: >> >> - significant amount of bits has another definition. I will bring there >> a mess of new defines with A1_ suffixes. >> >> - registers of this controller are located in two separate regions. It >> will give a lot of complications for 'axg-audio' to support this. >> >> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> --- >> drivers/clk/meson/Kconfig | 13 + >> drivers/clk/meson/Makefile | 1 + >> drivers/clk/meson/a1-audio.c | 556 +++++++++++++++++++++++++++++++++++ >> drivers/clk/meson/a1-audio.h | 58 ++++ >> 4 files changed, 628 insertions(+) >> create mode 100644 drivers/clk/meson/a1-audio.c >> create mode 100644 drivers/clk/meson/a1-audio.h >> >> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig >> index d6a2fa5f7e88..80c4a18c83d2 100644 >> --- a/drivers/clk/meson/Kconfig >> +++ b/drivers/clk/meson/Kconfig >> @@ -133,6 +133,19 @@ config COMMON_CLK_A1_PERIPHERALS >> device, A1 SoC Family. Say Y if you want A1 Peripherals clock >> controller to work. >> >> +config COMMON_CLK_A1_AUDIO >> + tristate "Amlogic A1 SoC Audio clock controller support" >> + depends on ARM64 >> + select COMMON_CLK_MESON_REGMAP >> + select COMMON_CLK_MESON_CLKC_UTILS >> + select COMMON_CLK_MESON_PHASE >> + select COMMON_CLK_MESON_SCLK_DIV >> + select COMMON_CLK_MESON_AUDIO_RSTC >> + help >> + Support for the Audio clock controller on Amlogic A113L based >> + device, A1 SoC Family. Say Y if you want A1 Audio clock controller >> + to work. >> + >> config COMMON_CLK_G12A >> tristate "G12 and SM1 SoC clock controllers support" >> depends on ARM64 >> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile >> index 88d94921a4dc..4968fc7ad555 100644 >> --- a/drivers/clk/meson/Makefile >> +++ b/drivers/clk/meson/Makefile >> @@ -20,6 +20,7 @@ obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o >> obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o >> obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o >> obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o >> +obj-$(CONFIG_COMMON_CLK_A1_AUDIO) += a1-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 >> diff --git a/drivers/clk/meson/a1-audio.c b/drivers/clk/meson/a1-audio.c >> new file mode 100644 >> index 000000000000..6039116c93ba >> --- /dev/null >> +++ b/drivers/clk/meson/a1-audio.c >> @@ -0,0 +1,556 @@ >> +// SPDX-License-Identifier: (GPL-2.0 OR MIT) >> +/* >> + * Copyright (c) 2024, SaluteDevices. All Rights Reserved. >> + * >> + * Author: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/clk-provider.h> >> +#include <linux/init.h> >> +#include <linux/of_device.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> +#include <linux/reset.h> >> +#include <linux/reset-controller.h> >> +#include <linux/slab.h> >> + >> +#include "meson-clkc-utils.h" >> +#include "meson-audio-rstc.h" >> +#include "clk-regmap.h" >> +#include "clk-phase.h" >> +#include "sclk-div.h" >> +#include "a1-audio.h" >> + >> +#define AUDIO_PDATA(_name) \ >> + ((const struct clk_parent_data[]) { { .hw = &(_name).hw } }) > > Not a fan - yet another level of macro. > >> + >> +#define AUDIO_MUX(_name, _reg, _mask, _shift, _pdata) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct clk_regmap_mux_data){ \ >> + .offset = AUDIO_REG_OFFSET(_reg), \ >> + .mask = (_mask), \ >> + .shift = (_shift), \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &clk_regmap_mux_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = ARRAY_SIZE(_pdata), \ >> + .flags = CLK_SET_RATE_PARENT, \ >> + }, \ >> +} >> + >> +#define AUDIO_DIV(_name, _reg, _shift, _width, _pdata) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct clk_regmap_div_data){ \ >> + .offset = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift), \ >> + .width = (_width), \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &clk_regmap_divider_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = 1, \ >> + .flags = CLK_SET_RATE_PARENT, \ >> + }, \ >> +} >> + >> +#define AUDIO_GATE(_name, _reg, _bit, _pdata) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct clk_regmap_gate_data){ \ >> + .offset = AUDIO_REG_OFFSET(_reg), \ >> + .bit_idx = (_bit), \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &clk_regmap_gate_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = 1, \ >> + .flags = CLK_SET_RATE_PARENT, \ >> + }, \ >> +} >> + >> +#define AUDIO_SCLK_DIV(_name, _reg, _div_shift, _div_width, \ >> + _hi_shift, _hi_width, _pdata, _set_rate_parent) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct meson_sclk_div_data) { \ >> + .div = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_div_shift), \ >> + .width = (_div_width), \ >> + }, \ >> + .hi = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_hi_shift), \ >> + .width = (_hi_width), \ >> + }, \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &meson_sclk_div_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = 1, \ >> + .flags = (_set_rate_parent) ? CLK_SET_RATE_PARENT : 0, \ > > Does not help readeability. Just pass the flag as axg-audio does. > >> + }, \ >> +} >> + >> +#define AUDIO_TRIPHASE(_name, _reg, _width, _shift0, _shift1, _shift2, \ >> + _pdata) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct meson_clk_triphase_data) { \ >> + .ph0 = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift0), \ >> + .width = (_width), \ >> + }, \ >> + .ph1 = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift1), \ >> + .width = (_width), \ >> + }, \ >> + .ph2 = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift2), \ >> + .width = (_width), \ >> + }, \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &meson_clk_triphase_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = 1, \ >> + .flags = CLK_SET_RATE_PARENT | CLK_DUTY_CYCLE_PARENT, \ >> + }, \ >> +} >> + >> +#define AUDIO_SCLK_WS(_name, _reg, _width, _shift_ph, _shift_ws, \ >> + _pdata) \ >> +static struct clk_regmap _name = { \ >> + .map = AUDIO_REG_MAP(_reg), \ >> + .data = &(struct meson_sclk_ws_inv_data) { \ >> + .ph = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift_ph), \ >> + .width = (_width), \ >> + }, \ >> + .ws = { \ >> + .reg_off = AUDIO_REG_OFFSET(_reg), \ >> + .shift = (_shift_ws), \ >> + .width = (_width), \ >> + }, \ >> + }, \ >> + .hw.init = &(struct clk_init_data) { \ >> + .name = #_name, \ >> + .ops = &meson_sclk_ws_inv_ops, \ >> + .parent_data = (_pdata), \ >> + .num_parents = 1, \ >> + .flags = CLK_SET_RATE_PARENT | CLK_DUTY_CYCLE_PARENT, \ >> + }, \ >> +} > > All the above does essentially the same things as the macro of > axg-audio, to some minor differences. Yet it is another set to maintain. > Except one thing... Here I keep memory identifier to which this clock belongs: .map = AUDIO_REG_MAP(_reg), It is workaround, but ->map the only common field in clk_regmap that could be used for this purpose. > I'd much prefer if you put the axg-audio macro in a header a re-used > those. There would a single set to maintain. You may then specialize the > included in the driver C file, to avoid redundant parameters > > Rework axg-audio to use clk_parent_data if you must, but not in the same > series please. > >> + >> +static const struct clk_parent_data a1_pclk_pdata[] = { >> + { .fw_name = "pclk", }, >> +}; >> + >> +AUDIO_GATE(audio_ddr_arb, AUDIO_CLK_GATE_EN0, 0, a1_pclk_pdata); >> +AUDIO_GATE(audio_tdmin_a, AUDIO_CLK_GATE_EN0, 1, a1_pclk_pdata); >> +AUDIO_GATE(audio_tdmin_b, AUDIO_CLK_GATE_EN0, 2, a1_pclk_pdata); >> +AUDIO_GATE(audio_tdmin_lb, AUDIO_CLK_GATE_EN0, 3, a1_pclk_pdata); >> +AUDIO_GATE(audio_loopback, AUDIO_CLK_GATE_EN0, 4, a1_pclk_pdata); >> +AUDIO_GATE(audio_tdmout_a, AUDIO_CLK_GATE_EN0, 5, a1_pclk_pdata); >> +AUDIO_GATE(audio_tdmout_b, AUDIO_CLK_GATE_EN0, 6, a1_pclk_pdata); >> +AUDIO_GATE(audio_frddr_a, AUDIO_CLK_GATE_EN0, 7, a1_pclk_pdata); >> +AUDIO_GATE(audio_frddr_b, AUDIO_CLK_GATE_EN0, 8, a1_pclk_pdata); >> +AUDIO_GATE(audio_toddr_a, AUDIO_CLK_GATE_EN0, 9, a1_pclk_pdata); >> +AUDIO_GATE(audio_toddr_b, AUDIO_CLK_GATE_EN0, 10, a1_pclk_pdata); >> +AUDIO_GATE(audio_spdifin, AUDIO_CLK_GATE_EN0, 11, a1_pclk_pdata); >> +AUDIO_GATE(audio_resample, AUDIO_CLK_GATE_EN0, 12, a1_pclk_pdata); >> +AUDIO_GATE(audio_eqdrc, AUDIO_CLK_GATE_EN0, 13, a1_pclk_pdata); >> +AUDIO_GATE(audio_audiolocker, AUDIO_CLK_GATE_EN0, 14, a1_pclk_pdata); > This is what I mean by redundant parameter ^ > Yep. I could define something like AUDIO_PCLK_GATE(). >> + >> +AUDIO_GATE(audio2_ddr_arb, AUDIO2_CLK_GATE_EN0, 0, a1_pclk_pdata); >> +AUDIO_GATE(audio2_pdm, AUDIO2_CLK_GATE_EN0, 1, a1_pclk_pdata); >> +AUDIO_GATE(audio2_tdmin_vad, AUDIO2_CLK_GATE_EN0, 2, a1_pclk_pdata); >> +AUDIO_GATE(audio2_toddr_vad, AUDIO2_CLK_GATE_EN0, 3, a1_pclk_pdata); >> +AUDIO_GATE(audio2_vad, AUDIO2_CLK_GATE_EN0, 4, a1_pclk_pdata); >> +AUDIO_GATE(audio2_audiotop, AUDIO2_CLK_GATE_EN0, 7, a1_pclk_pdata); >> + >> +static const struct clk_parent_data a1_mst_pdata[] = { >> + { .fw_name = "dds_in" }, >> + { .fw_name = "fclk_div2" }, >> + { .fw_name = "fclk_div3" }, >> + { .fw_name = "hifi_pll" }, >> + { .fw_name = "xtal" }, >> +}; >> + >> +#define AUDIO_MST_MCLK(_name, _reg) \ >> + AUDIO_MUX(_name##_mux, (_reg), 0x7, 24, a1_mst_pdata); \ >> + AUDIO_DIV(_name##_div, (_reg), 0, 16, \ >> + AUDIO_PDATA(_name##_mux)); \ >> + AUDIO_GATE(_name, (_reg), 31, AUDIO_PDATA(_name##_div)) >> + >> +AUDIO_MST_MCLK(audio_mst_a_mclk, AUDIO_MCLK_A_CTRL); >> +AUDIO_MST_MCLK(audio_mst_b_mclk, AUDIO_MCLK_B_CTRL); >> +AUDIO_MST_MCLK(audio_mst_c_mclk, AUDIO_MCLK_C_CTRL); >> +AUDIO_MST_MCLK(audio_mst_d_mclk, AUDIO_MCLK_D_CTRL); >> +AUDIO_MST_MCLK(audio_spdifin_clk, AUDIO_CLK_SPDIFIN_CTRL); >> +AUDIO_MST_MCLK(audio_eqdrc_clk, AUDIO_CLK_EQDRC_CTRL); >> + >> +AUDIO_MUX(audio_resample_clk_mux, AUDIO_CLK_RESAMPLE_CTRL, 0xf, 24, >> + a1_mst_pdata); >> +AUDIO_DIV(audio_resample_clk_div, AUDIO_CLK_RESAMPLE_CTRL, 0, 8, >> + AUDIO_PDATA(audio_resample_clk_mux)); >> +AUDIO_GATE(audio_resample_clk, AUDIO_CLK_RESAMPLE_CTRL, 31, >> + AUDIO_PDATA(audio_resample_clk_div)); >> + >> +AUDIO_MUX(audio_locker_in_clk_mux, AUDIO_CLK_LOCKER_CTRL, 0xf, 8, >> + a1_mst_pdata); >> +AUDIO_DIV(audio_locker_in_clk_div, AUDIO_CLK_LOCKER_CTRL, 0, 8, >> + AUDIO_PDATA(audio_locker_in_clk_mux)); >> +AUDIO_GATE(audio_locker_in_clk, AUDIO_CLK_LOCKER_CTRL, 15, >> + AUDIO_PDATA(audio_locker_in_clk_div)); >> + >> +AUDIO_MUX(audio_locker_out_clk_mux, AUDIO_CLK_LOCKER_CTRL, 0xf, 24, >> + a1_mst_pdata); >> +AUDIO_DIV(audio_locker_out_clk_div, AUDIO_CLK_LOCKER_CTRL, 16, 8, >> + AUDIO_PDATA(audio_locker_out_clk_mux)); >> +AUDIO_GATE(audio_locker_out_clk, AUDIO_CLK_LOCKER_CTRL, 31, >> + AUDIO_PDATA(audio_locker_out_clk_div)); >> + >> +AUDIO_MST_MCLK(audio2_vad_mclk, AUDIO2_MCLK_VAD_CTRL); >> +AUDIO_MST_MCLK(audio2_vad_clk, AUDIO2_CLK_VAD_CTRL); >> +AUDIO_MST_MCLK(audio2_pdm_dclk, AUDIO2_CLK_PDMIN_CTRL0); >> +AUDIO_MST_MCLK(audio2_pdm_sysclk, AUDIO2_CLK_PDMIN_CTRL1); >> + >> +#define AUDIO_MST_SCLK(_name, _reg0, _reg1, _pdata) \ >> + AUDIO_GATE(_name##_pre_en, (_reg0), 31, (_pdata)); \ >> + AUDIO_SCLK_DIV(_name##_div, (_reg0), 20, 10, 0, 0, \ >> + AUDIO_PDATA(_name##_pre_en), true); \ >> + AUDIO_GATE(_name##_post_en, (_reg0), 30, \ >> + AUDIO_PDATA(_name##_div)); \ >> + AUDIO_TRIPHASE(_name, (_reg1), 1, 0, 2, 4, \ >> + AUDIO_PDATA(_name##_post_en)) >> + > > Again, I'm not a fan of this many levels of macro. I can live with it > but certainly don't want the burden of reviewing and maintaining for > clock driver. AXG / G12 and A1 are obviously closely related, so make it common. > >> +#define AUDIO_MST_LRCLK(_name, _reg0, _reg1, _pdata) \ >> + AUDIO_SCLK_DIV(_name##_div, (_reg0), 0, 10, 10, 10, \ >> + (_pdata), false); \ >> + AUDIO_TRIPHASE(_name, (_reg1), 1, 1, 3, 5, \ >> + AUDIO_PDATA(_name##_div)) >> + >> +AUDIO_MST_SCLK(audio_mst_a_sclk, AUDIO_MST_A_SCLK_CTRL0, AUDIO_MST_A_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_a_mclk)); >> +AUDIO_MST_SCLK(audio_mst_b_sclk, AUDIO_MST_B_SCLK_CTRL0, AUDIO_MST_B_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_b_mclk)); >> +AUDIO_MST_SCLK(audio_mst_c_sclk, AUDIO_MST_C_SCLK_CTRL0, AUDIO_MST_C_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_c_mclk)); >> +AUDIO_MST_SCLK(audio_mst_d_sclk, AUDIO_MST_D_SCLK_CTRL0, AUDIO_MST_D_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_d_mclk)); >> + >> +AUDIO_MST_LRCLK(audio_mst_a_lrclk, AUDIO_MST_A_SCLK_CTRL0, AUDIO_MST_A_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_a_sclk_post_en)); >> +AUDIO_MST_LRCLK(audio_mst_b_lrclk, AUDIO_MST_B_SCLK_CTRL0, AUDIO_MST_B_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_b_sclk_post_en)); >> +AUDIO_MST_LRCLK(audio_mst_c_lrclk, AUDIO_MST_C_SCLK_CTRL0, AUDIO_MST_C_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_c_sclk_post_en)); >> +AUDIO_MST_LRCLK(audio_mst_d_lrclk, AUDIO_MST_D_SCLK_CTRL0, AUDIO_MST_D_SCLK_CTRL1, >> + AUDIO_PDATA(audio_mst_d_sclk_post_en)); >> + >> +static const struct clk_parent_data a1_mst_sclk_pdata[] = { >> + { .hw = &audio_mst_a_sclk.hw }, >> + { .hw = &audio_mst_b_sclk.hw }, >> + { .hw = &audio_mst_c_sclk.hw }, >> + { .hw = &audio_mst_d_sclk.hw }, >> + { .fw_name = "slv_sclk0" }, >> + { .fw_name = "slv_sclk1" }, >> + { .fw_name = "slv_sclk2" }, >> + { .fw_name = "slv_sclk3" }, >> + { .fw_name = "slv_sclk4" }, >> + { .fw_name = "slv_sclk5" }, >> + { .fw_name = "slv_sclk6" }, >> + { .fw_name = "slv_sclk7" }, >> + { .fw_name = "slv_sclk8" }, >> + { .fw_name = "slv_sclk9" }, >> +}; >> + >> +static const struct clk_parent_data a1_mst_lrclk_pdata[] = { >> + { .hw = &audio_mst_a_lrclk.hw }, >> + { .hw = &audio_mst_b_lrclk.hw }, >> + { .hw = &audio_mst_c_lrclk.hw }, >> + { .hw = &audio_mst_d_lrclk.hw }, >> + { .fw_name = "slv_lrclk0" }, >> + { .fw_name = "slv_lrclk1" }, >> + { .fw_name = "slv_lrclk2" }, >> + { .fw_name = "slv_lrclk3" }, >> + { .fw_name = "slv_lrclk4" }, >> + { .fw_name = "slv_lrclk5" }, >> + { .fw_name = "slv_lrclk6" }, >> + { .fw_name = "slv_lrclk7" }, >> + { .fw_name = "slv_lrclk8" }, >> + { .fw_name = "slv_lrclk9" }, >> +}; >> + >> +#define AUDIO_TDM_SCLK(_name, _reg) \ >> + AUDIO_MUX(_name##_mux, (_reg), 0xf, 24, a1_mst_sclk_pdata); \ >> + AUDIO_GATE(_name##_pre_en, (_reg), 31, \ >> + AUDIO_PDATA(_name##_mux)); \ >> + AUDIO_GATE(_name##_post_en, (_reg), 30, \ >> + AUDIO_PDATA(_name##_pre_en)); \ >> + AUDIO_SCLK_WS(_name, (_reg), 1, 29, 28, \ >> + AUDIO_PDATA(_name##_post_en)) >> + >> +#define AUDIO_TDM_LRCLK(_name, _reg) \ >> + AUDIO_MUX(_name, (_reg), 0xf, 20, a1_mst_lrclk_pdata) >> + >> +AUDIO_TDM_SCLK(audio_tdmin_a_sclk, AUDIO_CLK_TDMIN_A_CTRL); >> +AUDIO_TDM_SCLK(audio_tdmin_b_sclk, AUDIO_CLK_TDMIN_B_CTRL); >> +AUDIO_TDM_SCLK(audio_tdmin_lb_sclk, AUDIO_CLK_TDMIN_LB_CTRL); >> +AUDIO_TDM_SCLK(audio_tdmout_a_sclk, AUDIO_CLK_TDMOUT_A_CTRL); >> +AUDIO_TDM_SCLK(audio_tdmout_b_sclk, AUDIO_CLK_TDMOUT_B_CTRL); >> + >> +AUDIO_TDM_LRCLK(audio_tdmin_a_lrclk, AUDIO_CLK_TDMIN_A_CTRL); >> +AUDIO_TDM_LRCLK(audio_tdmin_b_lrclk, AUDIO_CLK_TDMIN_B_CTRL); >> +AUDIO_TDM_LRCLK(audio_tdmin_lb_lrclk, AUDIO_CLK_TDMIN_LB_CTRL); >> +AUDIO_TDM_LRCLK(audio_tdmout_a_lrclk, AUDIO_CLK_TDMOUT_A_CTRL); >> +AUDIO_TDM_LRCLK(audio_tdmout_b_lrclk, AUDIO_CLK_TDMOUT_B_CTRL); >> + >> +static struct clk_hw *a1_audio_hw_clks[] = { >> + [AUD_CLKID_DDR_ARB] = &audio_ddr_arb.hw, >> + [AUD_CLKID_TDMIN_A] = &audio_tdmin_a.hw, >> + [AUD_CLKID_TDMIN_B] = &audio_tdmin_b.hw, >> + [AUD_CLKID_TDMIN_LB] = &audio_tdmin_lb.hw, >> + [AUD_CLKID_LOOPBACK] = &audio_loopback.hw, >> + [AUD_CLKID_TDMOUT_A] = &audio_tdmout_a.hw, >> + [AUD_CLKID_TDMOUT_B] = &audio_tdmout_b.hw, >> + [AUD_CLKID_FRDDR_A] = &audio_frddr_a.hw, >> + [AUD_CLKID_FRDDR_B] = &audio_frddr_b.hw, >> + [AUD_CLKID_TODDR_A] = &audio_toddr_a.hw, >> + [AUD_CLKID_TODDR_B] = &audio_toddr_b.hw, >> + [AUD_CLKID_SPDIFIN] = &audio_spdifin.hw, >> + [AUD_CLKID_RESAMPLE] = &audio_resample.hw, >> + [AUD_CLKID_EQDRC] = &audio_eqdrc.hw, >> + [AUD_CLKID_LOCKER] = &audio_audiolocker.hw, >> + [AUD_CLKID_MST_A_MCLK_SEL] = &audio_mst_a_mclk_mux.hw, >> + [AUD_CLKID_MST_A_MCLK_DIV] = &audio_mst_a_mclk_div.hw, >> + [AUD_CLKID_MST_A_MCLK] = &audio_mst_a_mclk.hw, >> + [AUD_CLKID_MST_B_MCLK_SEL] = &audio_mst_b_mclk_mux.hw, >> + [AUD_CLKID_MST_B_MCLK_DIV] = &audio_mst_b_mclk_div.hw, >> + [AUD_CLKID_MST_B_MCLK] = &audio_mst_b_mclk.hw, >> + [AUD_CLKID_MST_C_MCLK_SEL] = &audio_mst_c_mclk_mux.hw, >> + [AUD_CLKID_MST_C_MCLK_DIV] = &audio_mst_c_mclk_div.hw, >> + [AUD_CLKID_MST_C_MCLK] = &audio_mst_c_mclk.hw, >> + [AUD_CLKID_MST_D_MCLK_SEL] = &audio_mst_d_mclk_mux.hw, >> + [AUD_CLKID_MST_D_MCLK_DIV] = &audio_mst_d_mclk_div.hw, >> + [AUD_CLKID_MST_D_MCLK] = &audio_mst_d_mclk.hw, >> + [AUD_CLKID_RESAMPLE_CLK_SEL] = &audio_resample_clk_mux.hw, >> + [AUD_CLKID_RESAMPLE_CLK_DIV] = &audio_resample_clk_div.hw, >> + [AUD_CLKID_RESAMPLE_CLK] = &audio_resample_clk.hw, >> + [AUD_CLKID_LOCKER_IN_CLK_SEL] = &audio_locker_in_clk_mux.hw, >> + [AUD_CLKID_LOCKER_IN_CLK_DIV] = &audio_locker_in_clk_div.hw, >> + [AUD_CLKID_LOCKER_IN_CLK] = &audio_locker_in_clk.hw, >> + [AUD_CLKID_LOCKER_OUT_CLK_SEL] = &audio_locker_out_clk_mux.hw, >> + [AUD_CLKID_LOCKER_OUT_CLK_DIV] = &audio_locker_out_clk_div.hw, >> + [AUD_CLKID_LOCKER_OUT_CLK] = &audio_locker_out_clk.hw, >> + [AUD_CLKID_SPDIFIN_CLK_SEL] = &audio_spdifin_clk_mux.hw, >> + [AUD_CLKID_SPDIFIN_CLK_DIV] = &audio_spdifin_clk_div.hw, >> + [AUD_CLKID_SPDIFIN_CLK] = &audio_spdifin_clk.hw, >> + [AUD_CLKID_EQDRC_CLK_SEL] = &audio_eqdrc_clk_mux.hw, >> + [AUD_CLKID_EQDRC_CLK_DIV] = &audio_eqdrc_clk_div.hw, >> + [AUD_CLKID_EQDRC_CLK] = &audio_eqdrc_clk.hw, >> + [AUD_CLKID_MST_A_SCLK_PRE_EN] = &audio_mst_a_sclk_pre_en.hw, >> + [AUD_CLKID_MST_A_SCLK_DIV] = &audio_mst_a_sclk_div.hw, >> + [AUD_CLKID_MST_A_SCLK_POST_EN] = &audio_mst_a_sclk_post_en.hw, >> + [AUD_CLKID_MST_A_SCLK] = &audio_mst_a_sclk.hw, >> + [AUD_CLKID_MST_B_SCLK_PRE_EN] = &audio_mst_b_sclk_pre_en.hw, >> + [AUD_CLKID_MST_B_SCLK_DIV] = &audio_mst_b_sclk_div.hw, >> + [AUD_CLKID_MST_B_SCLK_POST_EN] = &audio_mst_b_sclk_post_en.hw, >> + [AUD_CLKID_MST_B_SCLK] = &audio_mst_b_sclk.hw, >> + [AUD_CLKID_MST_C_SCLK_PRE_EN] = &audio_mst_c_sclk_pre_en.hw, >> + [AUD_CLKID_MST_C_SCLK_DIV] = &audio_mst_c_sclk_div.hw, >> + [AUD_CLKID_MST_C_SCLK_POST_EN] = &audio_mst_c_sclk_post_en.hw, >> + [AUD_CLKID_MST_C_SCLK] = &audio_mst_c_sclk.hw, >> + [AUD_CLKID_MST_D_SCLK_PRE_EN] = &audio_mst_d_sclk_pre_en.hw, >> + [AUD_CLKID_MST_D_SCLK_DIV] = &audio_mst_d_sclk_div.hw, >> + [AUD_CLKID_MST_D_SCLK_POST_EN] = &audio_mst_d_sclk_post_en.hw, >> + [AUD_CLKID_MST_D_SCLK] = &audio_mst_d_sclk.hw, >> + [AUD_CLKID_MST_A_LRCLK_DIV] = &audio_mst_a_lrclk_div.hw, >> + [AUD_CLKID_MST_A_LRCLK] = &audio_mst_a_lrclk.hw, >> + [AUD_CLKID_MST_B_LRCLK_DIV] = &audio_mst_b_lrclk_div.hw, >> + [AUD_CLKID_MST_B_LRCLK] = &audio_mst_b_lrclk.hw, >> + [AUD_CLKID_MST_C_LRCLK_DIV] = &audio_mst_c_lrclk_div.hw, >> + [AUD_CLKID_MST_C_LRCLK] = &audio_mst_c_lrclk.hw, >> + [AUD_CLKID_MST_D_LRCLK_DIV] = &audio_mst_d_lrclk_div.hw, >> + [AUD_CLKID_MST_D_LRCLK] = &audio_mst_d_lrclk.hw, >> + [AUD_CLKID_TDMIN_A_SCLK_SEL] = &audio_tdmin_a_sclk_mux.hw, >> + [AUD_CLKID_TDMIN_A_SCLK_PRE_EN] = &audio_tdmin_a_sclk_pre_en.hw, >> + [AUD_CLKID_TDMIN_A_SCLK_POST_EN] = &audio_tdmin_a_sclk_post_en.hw, >> + [AUD_CLKID_TDMIN_A_SCLK] = &audio_tdmin_a_sclk.hw, >> + [AUD_CLKID_TDMIN_A_LRCLK] = &audio_tdmin_a_lrclk.hw, >> + [AUD_CLKID_TDMIN_B_SCLK_SEL] = &audio_tdmin_b_sclk_mux.hw, >> + [AUD_CLKID_TDMIN_B_SCLK_PRE_EN] = &audio_tdmin_b_sclk_pre_en.hw, >> + [AUD_CLKID_TDMIN_B_SCLK_POST_EN] = &audio_tdmin_b_sclk_post_en.hw, >> + [AUD_CLKID_TDMIN_B_SCLK] = &audio_tdmin_b_sclk.hw, >> + [AUD_CLKID_TDMIN_B_LRCLK] = &audio_tdmin_b_lrclk.hw, >> + [AUD_CLKID_TDMIN_LB_SCLK_SEL] = &audio_tdmin_lb_sclk_mux.hw, >> + [AUD_CLKID_TDMIN_LB_SCLK_PRE_EN] = &audio_tdmin_lb_sclk_pre_en.hw, >> + [AUD_CLKID_TDMIN_LB_SCLK_POST_EN] = &audio_tdmin_lb_sclk_post_en.hw, >> + [AUD_CLKID_TDMIN_LB_SCLK] = &audio_tdmin_lb_sclk.hw, >> + [AUD_CLKID_TDMIN_LB_LRCLK] = &audio_tdmin_lb_lrclk.hw, >> + [AUD_CLKID_TDMOUT_A_SCLK_SEL] = &audio_tdmout_a_sclk_mux.hw, >> + [AUD_CLKID_TDMOUT_A_SCLK_PRE_EN] = &audio_tdmout_a_sclk_pre_en.hw, >> + [AUD_CLKID_TDMOUT_A_SCLK_POST_EN] = &audio_tdmout_a_sclk_post_en.hw, >> + [AUD_CLKID_TDMOUT_A_SCLK] = &audio_tdmout_a_sclk.hw, >> + [AUD_CLKID_TDMOUT_A_LRCLK] = &audio_tdmout_a_lrclk.hw, >> + [AUD_CLKID_TDMOUT_B_SCLK_SEL] = &audio_tdmout_b_sclk_mux.hw, >> + [AUD_CLKID_TDMOUT_B_SCLK_PRE_EN] = &audio_tdmout_b_sclk_pre_en.hw, >> + [AUD_CLKID_TDMOUT_B_SCLK_POST_EN] = &audio_tdmout_b_sclk_post_en.hw, >> + [AUD_CLKID_TDMOUT_B_SCLK] = &audio_tdmout_b_sclk.hw, >> + [AUD_CLKID_TDMOUT_B_LRCLK] = &audio_tdmout_b_lrclk.hw, >> + >> + [AUD2_CLKID_DDR_ARB] = &audio2_ddr_arb.hw, >> + [AUD2_CLKID_PDM] = &audio2_pdm.hw, >> + [AUD2_CLKID_TDMIN_VAD] = &audio2_tdmin_vad.hw, >> + [AUD2_CLKID_TODDR_VAD] = &audio2_toddr_vad.hw, >> + [AUD2_CLKID_VAD] = &audio2_vad.hw, >> + [AUD2_CLKID_AUDIOTOP] = &audio2_audiotop.hw, >> + [AUD2_CLKID_VAD_MCLK_SEL] = &audio2_vad_mclk_mux.hw, >> + [AUD2_CLKID_VAD_MCLK_DIV] = &audio2_vad_mclk_div.hw, >> + [AUD2_CLKID_VAD_MCLK] = &audio2_vad_mclk.hw, >> + [AUD2_CLKID_VAD_CLK_SEL] = &audio2_vad_clk_mux.hw, >> + [AUD2_CLKID_VAD_CLK_DIV] = &audio2_vad_clk_div.hw, >> + [AUD2_CLKID_VAD_CLK] = &audio2_vad_clk.hw, >> + [AUD2_CLKID_PDM_DCLK_SEL] = &audio2_pdm_dclk_mux.hw, >> + [AUD2_CLKID_PDM_DCLK_DIV] = &audio2_pdm_dclk_div.hw, >> + [AUD2_CLKID_PDM_DCLK] = &audio2_pdm_dclk.hw, >> + [AUD2_CLKID_PDM_SYSCLK_SEL] = &audio2_pdm_sysclk_mux.hw, >> + [AUD2_CLKID_PDM_SYSCLK_DIV] = &audio2_pdm_sysclk_div.hw, >> + [AUD2_CLKID_PDM_SYSCLK] = &audio2_pdm_sysclk.hw, >> +}; >> + >> +static struct meson_clk_hw_data a1_audio_clks = { >> + .hws = a1_audio_hw_clks, >> + .num = ARRAY_SIZE(a1_audio_hw_clks), >> +}; >> + >> +static struct regmap *a1_audio_map(struct platform_device *pdev, >> + unsigned int index) >> +{ >> + char name[32]; >> + const struct regmap_config cfg = { >> + .reg_bits = 32, >> + .val_bits = 32, >> + .reg_stride = 4, >> + .name = name, > > Not necessary > This implementation uses two regmaps, and this field allow to avoid errors like this: [ 0.145530] debugfs: Directory 'fe050000.audio-clock-controller' with parent 'regmap' already present! >> + }; >> + void __iomem *base; >> + >> + base = devm_platform_ioremap_resource(pdev, index); >> + if (IS_ERR(base)) >> + return base; >> + >> + scnprintf(name, sizeof(name), "%d", index); >> + return devm_regmap_init_mmio(&pdev->dev, base, &cfg); >> +} > > That is overengineered. Please keep it simple. Declare the regmap_config > as static const global, and do it like axg-audio please. > This only reason why it is not "static const" because I need to set unique name for each regmap. >> + >> +static int a1_register_clk(struct platform_device *pdev, >> + struct regmap *map0, struct regmap *map1, >> + struct clk_hw *hw) >> +{ >> + struct clk_regmap *clk = container_of(hw, struct clk_regmap, hw); >> + >> + if (!hw) >> + return 0; >> + >> + switch ((unsigned long)clk->map) { >> + case AUDIO_RANGE_0: >> + clk->map = map0; >> + break; >> + case AUDIO_RANGE_1: >> + clk->map = map1; >> + break; > > ... fishy > >> + default: >> + WARN_ON(1); >> + return -EINVAL; >> + } >> + >> + return devm_clk_hw_register(&pdev->dev, hw); >> +} >> + >> +static int a1_audio_clkc_probe(struct platform_device *pdev) >> +{ >> + struct regmap *map0, *map1; >> + struct clk *clk; >> + unsigned int i; >> + int ret; >> + >> + clk = devm_clk_get_enabled(&pdev->dev, "pclk"); >> + if (WARN_ON(IS_ERR(clk))) >> + return PTR_ERR(clk); >> + >> + map0 = a1_audio_map(pdev, 0); >> + if (IS_ERR(map0)) >> + return PTR_ERR(map0); >> + >> + map1 = a1_audio_map(pdev, 1); >> + if (IS_ERR(map1)) >> + return PTR_ERR(map1); > > No - Looks to me you just have two clock controllers you are trying > force into one. > See the begining. >> + >> + /* >> + * Register and enable AUD2_CLKID_AUDIOTOP clock first. Unless >> + * it is enabled any read/write to 'map0' hangs the CPU. >> + */ >> + >> + ret = a1_register_clk(pdev, map0, map1, >> + a1_audio_clks.hws[AUD2_CLKID_AUDIOTOP]); >> + if (ret) >> + return ret; >> + >> + ret = clk_prepare_enable(a1_audio_clks.hws[AUD2_CLKID_AUDIOTOP]->clk); >> + if (ret) >> + return ret; > > Again, this shows 2 devices. The one related to your 'map0' should > request AUD2_CLKID_AUDIOTOP as input and enable it right away. > See the begining. >> + >> + for (i = 0; i < a1_audio_clks.num; i++) { >> + if (i == AUD2_CLKID_AUDIOTOP) >> + continue; >> + >> + ret = a1_register_clk(pdev, map0, map1, a1_audio_clks.hws[i]); >> + if (ret) >> + return ret; >> + } >> + >> + ret = devm_of_clk_add_hw_provider(&pdev->dev, meson_clk_hw_get, >> + &a1_audio_clks); >> + if (ret) >> + return ret; >> + >> + BUILD_BUG_ON((unsigned long)AUDIO_REG_MAP(AUDIO_SW_RESET0) != >> + AUDIO_RANGE_0); > > Why is that necessary ? > A little paranoia. Here AUDIO_SW_RESET0 is handled as map0's register, and I want to assert it. >> + return meson_audio_rstc_register(&pdev->dev, map0, >> + AUDIO_REG_OFFSET(AUDIO_SW_RESET0), 32); >> +} >> + >> +static const struct of_device_id a1_audio_clkc_match_table[] = { >> + { .compatible = "amlogic,a1-audio-clkc", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, a1_audio_clkc_match_table); >> + >> +static struct platform_driver a1_audio_clkc_driver = { >> + .probe = a1_audio_clkc_probe, >> + .driver = { >> + .name = "a1-audio-clkc", >> + .of_match_table = a1_audio_clkc_match_table, >> + }, >> +}; >> +module_platform_driver(a1_audio_clkc_driver); >> + >> +MODULE_DESCRIPTION("Amlogic A1 Audio Clock driver"); >> +MODULE_AUTHOR("Jan Dakinevich <jan.dakinevich@salutedevices.com>"); >> +MODULE_LICENSE("GPL"); >> diff --git a/drivers/clk/meson/a1-audio.h b/drivers/clk/meson/a1-audio.h >> new file mode 100644 >> index 000000000000..f994e87276cd >> --- /dev/null >> +++ b/drivers/clk/meson/a1-audio.h >> @@ -0,0 +1,58 @@ >> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */ >> +/* >> + * Copyright (c) 2024, SaluteDevices. All Rights Reserved. >> + * >> + * Author: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> + */ >> + >> +#ifndef __A1_AUDIO_H >> +#define __A1_AUDIO_H >> + >> +#define AUDIO_RANGE_0 0xa >> +#define AUDIO_RANGE_1 0xb >> +#define AUDIO_RANGE_SHIFT 16 >> + >> +#define AUDIO_REG(_range, _offset) \ >> + (((_range) << AUDIO_RANGE_SHIFT) + (_offset)) >> + >> +#define AUDIO_REG_OFFSET(_reg) \ >> + ((_reg) & ((1 << AUDIO_RANGE_SHIFT) - 1)) >> + >> +#define AUDIO_REG_MAP(_reg) \ >> + ((void *)((_reg) >> AUDIO_RANGE_SHIFT)) > > That is seriouly overengineered. > The following are offset. Just write what they are. > This is all in order to keep range's identifier together with offset and then use it to store the identifier in clk_regmaps. > There is not reason to put that into a header. It is only going to be > used by a single driver. > >> + >> +#define AUDIO_CLK_GATE_EN0 AUDIO_REG(AUDIO_RANGE_0, 0x000) >> +#define AUDIO_MCLK_A_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x008) >> +#define AUDIO_MCLK_B_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x00c) >> +#define AUDIO_MCLK_C_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x010) >> +#define AUDIO_MCLK_D_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x014) >> +#define AUDIO_MCLK_E_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x018) >> +#define AUDIO_MCLK_F_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x01c) >> +#define AUDIO_SW_RESET0 AUDIO_REG(AUDIO_RANGE_0, 0x028) >> +#define AUDIO_MST_A_SCLK_CTRL0 AUDIO_REG(AUDIO_RANGE_0, 0x040) >> +#define AUDIO_MST_A_SCLK_CTRL1 AUDIO_REG(AUDIO_RANGE_0, 0x044) >> +#define AUDIO_MST_B_SCLK_CTRL0 AUDIO_REG(AUDIO_RANGE_0, 0x048) >> +#define AUDIO_MST_B_SCLK_CTRL1 AUDIO_REG(AUDIO_RANGE_0, 0x04c) >> +#define AUDIO_MST_C_SCLK_CTRL0 AUDIO_REG(AUDIO_RANGE_0, 0x050) >> +#define AUDIO_MST_C_SCLK_CTRL1 AUDIO_REG(AUDIO_RANGE_0, 0x054) >> +#define AUDIO_MST_D_SCLK_CTRL0 AUDIO_REG(AUDIO_RANGE_0, 0x058) >> +#define AUDIO_MST_D_SCLK_CTRL1 AUDIO_REG(AUDIO_RANGE_0, 0x05c) >> +#define AUDIO_CLK_TDMIN_A_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x080) >> +#define AUDIO_CLK_TDMIN_B_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x084) >> +#define AUDIO_CLK_TDMIN_LB_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x08c) >> +#define AUDIO_CLK_TDMOUT_A_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x090) >> +#define AUDIO_CLK_TDMOUT_B_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x094) >> +#define AUDIO_CLK_SPDIFIN_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x09c) >> +#define AUDIO_CLK_RESAMPLE_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x0a4) >> +#define AUDIO_CLK_LOCKER_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x0a8) >> +#define AUDIO_CLK_EQDRC_CTRL AUDIO_REG(AUDIO_RANGE_0, 0x0c0) >> + >> +#define AUDIO2_CLK_GATE_EN0 AUDIO_REG(AUDIO_RANGE_1, 0x00c) >> +#define AUDIO2_MCLK_VAD_CTRL AUDIO_REG(AUDIO_RANGE_1, 0x040) >> +#define AUDIO2_CLK_VAD_CTRL AUDIO_REG(AUDIO_RANGE_1, 0x044) >> +#define AUDIO2_CLK_PDMIN_CTRL0 AUDIO_REG(AUDIO_RANGE_1, 0x058) >> +#define AUDIO2_CLK_PDMIN_CTRL1 AUDIO_REG(AUDIO_RANGE_1, 0x05c) >> + >> +#include <dt-bindings/clock/amlogic,a1-audio-clkc.h> >> + >> +#endif /* __A1_AUDIO_H */ > > -- Best regards Jan Dakinevich
On 3/18/24 13:55, Jerome Brunet wrote: > > On Sun 17 Mar 2024 at 18:52, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> On 3/15/24 13:22, Jerome Brunet wrote: >>> >>> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 15/03/2024 00:21, Jan Dakinevich wrote: >>>>> This option allow to redefine the rate of DSP system clock. >>>> >>>> And why is it suitable for bindings? Describe the hardware, not what you >>>> want to do in the driver. >>>> >>>>> >>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>>> --- >>>>> Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>> index df21dd72fc65..d2f23a59a6b6 100644 >>>>> --- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>> +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>> @@ -40,6 +40,10 @@ properties: >>>>> resets: >>>>> maxItems: 1 >>>>> >>>>> + sysrate: >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + description: redefine rate of DSP system clock >>>> >>>> No vendor prefix, so is it a generic property? Also, missing unit >>>> suffix, but more importantly I don't understand why this is a property >>>> of hardware. >>> >>> +1. >>> >>> The appropriate way to set rate of the clock before the driver take over >>> is 'assigned-rate', if you need to customize this for different >>> platform. >>> >> >> It would be great, but it doesn't work. Below, is what I want to see: >> >> assigned-clocks = >> <&clkc_audio AUD2_CLKID_PDM_SYSCLK_SEL>, >> <&clkc_audio AUD2_CLKID_PDM_SYSCLK_DIV>; >> assigned-clock-parents = >> <&clkc_pll CLKID_FCLK_DIV3>, >> <0>; >> assigned-clock-rates = >> <0>, >> <256000000>; >> >> But regardles of this declaration, PDM's driver unconditionally sets >> sysclk'rate to 250MHz and throws away everything that was configured >> before, reparents audio2_pdm_sysclk_mux to hifi_pll and changes >> hifi_pll's rate. >> >> This value 250MHz is declared here: >> >> static const struct axg_pdm_cfg axg_pdm_config = { >> .filters = &axg_default_filters, >> .sys_rate = 250000000, >> }; >> >> The property 'sysrate' is intended to redefine hardcoded 'sys_rate' >> value in 'axg_pdm_config'. > > What is stopping you from removing that from the driver and adding > assigned-rate to 250M is the existing platform ? > Ok, in next version I will try to remove this unconditional setting of rate that spoils my clock hierarchy. >> >>> Then you don't have to deal with it in the device driver. >>> >>>> >>>> Best regards, >>>> Krzysztof >>> >>> > > -- Best regards Jan Dakinevich
On 3/18/24 15:19, Jerome Brunet wrote: > > On Mon 18 Mar 2024 at 11:55, Jerome Brunet <jbrunet@baylibre.com> wrote: > >> On Sun 17 Mar 2024 at 18:52, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: >> >>> On 3/15/24 13:22, Jerome Brunet wrote: >>>> >>>> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>>> On 15/03/2024 00:21, Jan Dakinevich wrote: >>>>>> This option allow to redefine the rate of DSP system clock. >>>>> >>>>> And why is it suitable for bindings? Describe the hardware, not what you >>>>> want to do in the driver. >>>>> >>>>>> >>>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>>>> --- >>>>>> Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>>> index df21dd72fc65..d2f23a59a6b6 100644 >>>>>> --- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>>> +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml >>>>>> @@ -40,6 +40,10 @@ properties: >>>>>> resets: >>>>>> maxItems: 1 >>>>>> >>>>>> + sysrate: >>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>> + description: redefine rate of DSP system clock >>>>> >>>>> No vendor prefix, so is it a generic property? Also, missing unit >>>>> suffix, but more importantly I don't understand why this is a property >>>>> of hardware. >>>> >>>> +1. >>>> >>>> The appropriate way to set rate of the clock before the driver take over >>>> is 'assigned-rate', if you need to customize this for different >>>> platform. >>>> >>> >>> It would be great, but it doesn't work. Below, is what I want to see: >>> >>> assigned-clocks = >>> <&clkc_audio AUD2_CLKID_PDM_SYSCLK_SEL>, >>> <&clkc_audio AUD2_CLKID_PDM_SYSCLK_DIV>; >>> assigned-clock-parents = >>> <&clkc_pll CLKID_FCLK_DIV3>, >>> <0>; >>> assigned-clock-rates = >>> <0>, >>> <256000000>; >>> >>> But regardles of this declaration, PDM's driver unconditionally sets >>> sysclk'rate to 250MHz and throws away everything that was configured >>> before, reparents audio2_pdm_sysclk_mux to hifi_pll and changes >>> hifi_pll's rate. >>> >>> This value 250MHz is declared here: >>> >>> static const struct axg_pdm_cfg axg_pdm_config = { >>> .filters = &axg_default_filters, >>> .sys_rate = 250000000, >>> }; >>> >>> The property 'sysrate' is intended to redefine hardcoded 'sys_rate' >>> value in 'axg_pdm_config'. >> >> What is stopping you from removing that from the driver and adding >> assigned-rate to 250M is the existing platform ? > > ... Also, considering how PDM does work, I'm not sure I get the point of > the doing all this to go from 250MHz to 256Mhz. > The point is to use fclk_div3 clock as source for PDM's sysclock and keep hiff_pll clock free for TDM. Because, I can get 256MHz from any hifi_pll and fclk_div3, but only hifi_pll is able to provide accurate 48kHz (after several divider). > PDM value is sampled at ~75% of the half period. That clock basically > feeds a counter and the threshold is adjusted based on the clock rate. > > So there is no need to change the rate. Changing it is only necessary > when the captured audio rate is extremely slow (<8kHz) and the counter > may overflow. The driver already adjust this automatically. > > So changing the input rate from 250MHz to 256MHz should not make any > difference. > Thank you for the explanation. >> >>> >>>> Then you don't have to deal with it in the device driver. >>>> >>>>> >>>>> Best regards, >>>>> Krzysztof >>>> >>>> > > -- Best regards Jan Dakinevich
On 3/18/24 13:46, Jerome Brunet wrote: > > On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> A1's internal codec is very close to t9015. The main difference, that it >> has ADC. This commit introduces support for capturing from it. > > This is mis-leading. > > It does not look like the change is A1 specific but rather a extension > of the support for t9015. It also mixes several different topics like line > configuration, capture support, etc ... > First, it is not only extentsion. Some bits are changed comparing to existing t9015, so new compatible string is still required. Second, I don't know anything about about ADC in t9015 on other SoCs and even don't sure that it exist there (may be I am inattentive, but I'm unable to find audio input pin on sm1/g12a's pinout). > Again, the t9015 changes should be a separated series from the rest, and > there should be one patch per topic. > > As Mark, if something is meant to be configured based on the HW layout, > then there a good change a kcontrol is not appropriate, and this should > rather be part of the platform description, like DT. > > It was also suggested here: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/meson/t9015.c?h=v6.8#n298 > Ok. By the way, on a1 LINEOUT_CFG would have another value. >> >> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> --- >> sound/soc/meson/t9015.c | 259 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 259 insertions(+) >> >> diff --git a/sound/soc/meson/t9015.c b/sound/soc/meson/t9015.c >> index 48f6767bd858..365955bfeb78 100644 >> --- a/sound/soc/meson/t9015.c >> +++ b/sound/soc/meson/t9015.c >> @@ -19,16 +19,33 @@ >> #define LOLP_EN 3 >> #define DACR_EN 4 >> #define DACL_EN 5 >> +#define ADCR_EN 6 >> +#define ADCL_EN 7 >> +#define PGAR_ZCD_EN 8 >> +#define PGAL_ZCD_EN 9 >> +#define PGAR_EN 10 >> +#define PGAL_EN 11 >> +#define ADCR_INV 16 >> +#define ADCL_INV 17 >> +#define ADCR_SRC 18 >> +#define ADCL_SRC 19 >> #define DACR_INV 20 >> #define DACL_INV 21 >> #define DACR_SRC 22 >> #define DACL_SRC 23 >> +#define ADC_DEM_EN 26 >> +#define ADC_FILTER_MODE 28 >> +#define ADC_FILTER_EN 29 >> #define REFP_BUF_EN BIT(12) >> #define BIAS_CURRENT_EN BIT(13) >> #define VMID_GEN_FAST BIT(14) >> #define VMID_GEN_EN BIT(15) >> #define I2S_MODE BIT(30) >> #define VOL_CTRL0 0x04 >> +#define PGAR_VC 0 >> +#define PGAL_VC 8 >> +#define ADCR_VC 16 >> +#define ADCL_VC 24 >> #define GAIN_H 31 >> #define GAIN_L 23 >> #define VOL_CTRL1 0x08 >> @@ -46,6 +63,28 @@ >> #define LOLN_POL 8 >> #define LOLP_POL 12 >> #define POWER_CFG 0x10 >> +#define LINEIN_CFG 0x14 >> +#define MICBIAS_LEVEL 0 >> +#define MICBIAS_EN 3 >> +#define PGAR_CTVMN 8 >> +#define PGAR_CTVMP 9 >> +#define PGAL_CTVMN 10 >> +#define PGAL_CTVMP 11 >> +#define PGAR_CTVIN 12 >> +#define PGAR_CTVIP 13 >> +#define PGAL_CTVIN 14 >> +#define PGAL_CTVIP 15 >> + >> +#define PGAR_MASK (BIT(PGAR_CTVMP) | BIT(PGAR_CTVMN) | \ >> + BIT(PGAR_CTVIP) | BIT(PGAR_CTVIN)) >> +#define PGAR_DIFF (BIT(PGAR_CTVIP) | BIT(PGAR_CTVIN)) >> +#define PGAR_POSITIVE (BIT(PGAR_CTVIP) | BIT(PGAR_CTVMN)) >> +#define PGAR_NEGATIVE (BIT(PGAR_CTVIN) | BIT(PGAR_CTVMP)) >> +#define PGAL_MASK (BIT(PGAL_CTVMP) | BIT(PGAL_CTVMN) | \ >> + BIT(PGAL_CTVIP) | BIT(PGAL_CTVIN)) >> +#define PGAL_DIFF (BIT(PGAL_CTVIP) | BIT(PGAL_CTVIN)) >> +#define PGAL_POSITIVE (BIT(PGAL_CTVIP) | BIT(PGAL_CTVMN)) >> +#define PGAL_NEGATIVE (BIT(PGAL_CTVIN) | BIT(PGAL_CTVMP)) >> >> struct t9015 { >> struct regulator *avdd; >> @@ -103,6 +142,31 @@ static struct snd_soc_dai_driver t9015_dai = { >> .ops = &t9015_dai_ops, >> }; >> >> +static struct snd_soc_dai_driver a1_t9015_dai = { >> + .name = "t9015-hifi", >> + .playback = { >> + .stream_name = "Playback", >> + .channels_min = 1, >> + .channels_max = 2, >> + .rates = SNDRV_PCM_RATE_8000_96000, >> + .formats = (SNDRV_PCM_FMTBIT_S8 | >> + SNDRV_PCM_FMTBIT_S16_LE | >> + SNDRV_PCM_FMTBIT_S20_LE | >> + SNDRV_PCM_FMTBIT_S24_LE), >> + }, >> + .capture = { >> + .stream_name = "Capture", >> + .channels_min = 1, >> + .channels_max = 2, >> + .rates = SNDRV_PCM_RATE_8000_96000, >> + .formats = (SNDRV_PCM_FMTBIT_S8 | >> + SNDRV_PCM_FMTBIT_S16_LE | >> + SNDRV_PCM_FMTBIT_S20_LE | >> + SNDRV_PCM_FMTBIT_S24_LE), >> + }, >> + .ops = &t9015_dai_ops, >> +}; >> + >> static const DECLARE_TLV_DB_MINMAX_MUTE(dac_vol_tlv, -9525, 0); >> >> static const char * const ramp_rate_txt[] = { "Fast", "Slow" }; >> @@ -179,6 +243,166 @@ static const struct snd_soc_dapm_route t9015_dapm_routes[] = { >> { "LOLP", NULL, "Left+ Driver", }, >> }; >> >> +static const char * const a1_right_driver_txt[] = { "None", "Right DAC", >> + "Left DAC Inverted" }; >> +static const unsigned int a1_right_driver_values[] = { 0, 2, 4 }; >> + >> +static const char * const a1_left_driver_txt[] = { "None", "Left DAC", >> + "Right DAC Inverted" }; >> +static const unsigned int a1_left_driver_values[] = { 0, 2, 4 }; >> + >> +static SOC_VALUE_ENUM_SINGLE_DECL(a1_right_driver, LINEOUT_CFG, 12, 0x7, >> + a1_right_driver_txt, a1_right_driver_values); >> +static SOC_VALUE_ENUM_SINGLE_DECL(a1_left_driver, LINEOUT_CFG, 4, 0x7, >> + a1_left_driver_txt, a1_left_driver_values); >> + >> +static const struct snd_kcontrol_new a1_right_driver_mux = >> + SOC_DAPM_ENUM("Right Driver+ Source", a1_right_driver); >> +static const struct snd_kcontrol_new a1_left_driver_mux = >> + SOC_DAPM_ENUM("Left Driver+ Source", a1_left_driver); >> + >> +static const DECLARE_TLV_DB_MINMAX_MUTE(a1_adc_vol_tlv, -29625, 0); >> +static const DECLARE_TLV_DB_MINMAX_MUTE(a1_adc_pga_vol_tlv, -1200, 0); >> + >> +static const char * const a1_adc_right_txt[] = { "Right", "Left" }; >> +static SOC_ENUM_SINGLE_DECL(a1_adc_right, BLOCK_EN, ADCR_SRC, a1_adc_right_txt); >> + >> +static const char * const a1_adc_left_txt[] = { "Left", "Right" }; >> +static SOC_ENUM_SINGLE_DECL(a1_adc_left, BLOCK_EN, ADCL_SRC, a1_adc_left_txt); >> + >> +static const struct snd_kcontrol_new a1_adc_right_mux = >> + SOC_DAPM_ENUM("ADC Right Source", a1_adc_right); >> +static const struct snd_kcontrol_new a1_adc_left_mux = >> + SOC_DAPM_ENUM("ADC Left Source", a1_adc_left); >> + >> +static const char * const a1_adc_filter_mode_txt[] = { "Voice", "HiFi"}; >> +static SOC_ENUM_SINGLE_DECL(a1_adc_filter_mode, BLOCK_EN, ADC_FILTER_MODE, >> + a1_adc_filter_mode_txt); >> + >> +static const char * const a1_adc_mic_bias_level_txt[] = { "2.0V", "2.1V", >> + "2.3V", "2.5V", "2.8V" }; >> +static const unsigned int a1_adc_mic_bias_level_values[] = { 0, 1, 2, 3, 7 }; >> +static SOC_VALUE_ENUM_SINGLE_DECL(a1_adc_mic_bias_level, >> + LINEIN_CFG, MICBIAS_LEVEL, 0x7, >> + a1_adc_mic_bias_level_txt, >> + a1_adc_mic_bias_level_values); >> + >> +static const char * const a1_adc_pga_txt[] = { "None", "Differential", >> + "Positive", "Negative" }; >> +static const unsigned int a1_adc_pga_right_values[] = { 0, PGAR_DIFF, >> + PGAR_POSITIVE, PGAR_NEGATIVE }; >> +static const unsigned int a1_adc_pga_left_values[] = { 0, PGAL_DIFF, >> + PGAL_POSITIVE, PGAL_NEGATIVE }; >> + >> +static SOC_VALUE_ENUM_SINGLE_DECL(a1_adc_pga_right, LINEIN_CFG, 0, PGAR_MASK, >> + a1_adc_pga_txt, a1_adc_pga_right_values); >> +static SOC_VALUE_ENUM_SINGLE_DECL(a1_adc_pga_left, LINEIN_CFG, 0, PGAL_MASK, >> + a1_adc_pga_txt, a1_adc_pga_left_values); >> + >> +static const struct snd_kcontrol_new a1_adc_pga_right_mux = >> + SOC_DAPM_ENUM("ADC PGA Right Source", a1_adc_pga_right); >> +static const struct snd_kcontrol_new a1_adc_pga_left_mux = >> + SOC_DAPM_ENUM("ADC PGA Left Source", a1_adc_pga_left); >> + >> +static const struct snd_kcontrol_new a1_t9015_snd_controls[] = { >> + /* Volume Controls */ >> + SOC_ENUM("Playback Channel Mode", mono_enum), >> + SOC_SINGLE("Playback Switch", VOL_CTRL1, DAC_SOFT_MUTE, 1, 1), >> + SOC_DOUBLE_TLV("Playback Volume", VOL_CTRL1, DACL_VC, DACR_VC, >> + 0xff, 0, dac_vol_tlv), >> + >> + /* Ramp Controls */ >> + SOC_ENUM("Ramp Rate", ramp_rate_enum), >> + SOC_SINGLE("Volume Ramp Switch", VOL_CTRL1, VC_RAMP_MODE, 1, 0), >> + SOC_SINGLE("Mute Ramp Switch", VOL_CTRL1, MUTE_MODE, 1, 0), >> + SOC_SINGLE("Unmute Ramp Switch", VOL_CTRL1, UNMUTE_MODE, 1, 0), >> + >> + /* ADC Controls */ >> + SOC_DOUBLE_TLV("ADC Volume", VOL_CTRL0, ADCL_VC, ADCR_VC, >> + 0x7f, 0, a1_adc_vol_tlv), >> + SOC_SINGLE("ADC Filter Switch", BLOCK_EN, ADC_FILTER_EN, 1, 0), >> + SOC_ENUM("ADC Filter Mode", a1_adc_filter_mode), >> + SOC_SINGLE("ADC Mic Bias Switch", LINEIN_CFG, MICBIAS_EN, 1, 0), >> + SOC_ENUM("ADC Mic Bias Level", a1_adc_mic_bias_level), >> + SOC_SINGLE("ADC DEM Switch", BLOCK_EN, ADC_DEM_EN, 1, 0), >> + SOC_DOUBLE_TLV("ADC PGA Volume", VOL_CTRL0, PGAR_VC, PGAL_VC, >> + 0x1f, 0, a1_adc_pga_vol_tlv), >> + SOC_DOUBLE("ADC PGA Zero Cross-detection Switch", BLOCK_EN, >> + PGAL_ZCD_EN, PGAR_ZCD_EN, 1, 0), >> +}; >> + >> +static const struct snd_soc_dapm_widget a1_t9015_dapm_widgets[] = { >> + SND_SOC_DAPM_AIF_IN("Right IN", NULL, 0, SND_SOC_NOPM, 0, 0), >> + SND_SOC_DAPM_AIF_IN("Left IN", NULL, 0, SND_SOC_NOPM, 0, 0), >> + SND_SOC_DAPM_MUX("Right DAC Sel", SND_SOC_NOPM, 0, 0, >> + &t9015_right_dac_mux), >> + SND_SOC_DAPM_MUX("Left DAC Sel", SND_SOC_NOPM, 0, 0, >> + &t9015_left_dac_mux), >> + SND_SOC_DAPM_DAC("Right DAC", NULL, BLOCK_EN, DACR_EN, 0), >> + SND_SOC_DAPM_DAC("Left DAC", NULL, BLOCK_EN, DACL_EN, 0), >> + SND_SOC_DAPM_MUX("Right+ Driver Sel", SND_SOC_NOPM, 0, 0, >> + &a1_right_driver_mux), >> + SND_SOC_DAPM_MUX("Left+ Driver Sel", SND_SOC_NOPM, 0, 0, >> + &a1_left_driver_mux), >> + SND_SOC_DAPM_OUT_DRV("Right+ Driver", BLOCK_EN, LORP_EN, 0, NULL, 0), >> + SND_SOC_DAPM_OUT_DRV("Left+ Driver", BLOCK_EN, LOLP_EN, 0, NULL, 0), >> + SND_SOC_DAPM_OUTPUT("LORP"), >> + SND_SOC_DAPM_OUTPUT("LOLP"), >> + >> + SND_SOC_DAPM_INPUT("ADC IN Right"), >> + SND_SOC_DAPM_INPUT("ADC IN Left"), >> + SND_SOC_DAPM_MUX("ADC PGA Right Sel", SND_SOC_NOPM, 0, 0, >> + &a1_adc_pga_right_mux), >> + SND_SOC_DAPM_MUX("ADC PGA Left Sel", SND_SOC_NOPM, 0, 0, >> + &a1_adc_pga_left_mux), >> + SND_SOC_DAPM_PGA("ADC PGA Right", BLOCK_EN, PGAR_EN, 0, NULL, 0), >> + SND_SOC_DAPM_PGA("ADC PGA Left", BLOCK_EN, PGAL_EN, 0, NULL, 0), >> + SND_SOC_DAPM_ADC("ADC Right", NULL, BLOCK_EN, ADCR_EN, 0), >> + SND_SOC_DAPM_ADC("ADC Left", NULL, BLOCK_EN, ADCL_EN, 0), >> + SND_SOC_DAPM_MUX("ADC Right Sel", SND_SOC_NOPM, 0, 0, &a1_adc_right_mux), >> + SND_SOC_DAPM_MUX("ADC Left Sel", SND_SOC_NOPM, 0, 0, &a1_adc_left_mux), >> + SND_SOC_DAPM_AIF_OUT("ADC OUT Right", NULL, 0, SND_SOC_NOPM, 0, 0), >> + SND_SOC_DAPM_AIF_OUT("ADC OUT Left", NULL, 0, SND_SOC_NOPM, 0, 0), >> +}; >> + >> +static const struct snd_soc_dapm_route a1_t9015_dapm_routes[] = { >> + { "Right IN", NULL, "Playback" }, >> + { "Left IN", NULL, "Playback" }, >> + { "Right DAC Sel", "Right", "Right IN" }, >> + { "Right DAC Sel", "Left", "Left IN" }, >> + { "Left DAC Sel", "Right", "Right IN" }, >> + { "Left DAC Sel", "Left", "Left IN" }, >> + { "Right DAC", NULL, "Right DAC Sel" }, >> + { "Left DAC", NULL, "Left DAC Sel" }, >> + { "Right+ Driver Sel", "Right DAC", "Right DAC" }, >> + { "Right+ Driver Sel", "Left DAC Inverted", "Right DAC" }, >> + { "Left+ Driver Sel", "Left DAC", "Left DAC" }, >> + { "Left+ Driver Sel", "Right DAC Inverted", "Left DAC" }, >> + { "Right+ Driver", NULL, "Right+ Driver Sel" }, >> + { "Left+ Driver", NULL, "Left+ Driver Sel" }, >> + { "LORP", NULL, "Right+ Driver", }, >> + { "LOLP", NULL, "Left+ Driver", }, >> + >> + { "ADC PGA Right Sel", "Differential", "ADC IN Right" }, >> + { "ADC PGA Right Sel", "Positive", "ADC IN Right" }, >> + { "ADC PGA Right Sel", "Negative", "ADC IN Right" }, >> + { "ADC PGA Left Sel", "Differential", "ADC IN Left" }, >> + { "ADC PGA Left Sel", "Positive", "ADC IN Left" }, >> + { "ADC PGA Left Sel", "Negative", "ADC IN Left" }, >> + { "ADC PGA Right", NULL, "ADC PGA Right Sel" }, >> + { "ADC PGA Left", NULL, "ADC PGA Left Sel" }, >> + { "ADC Right", NULL, "ADC PGA Right" }, >> + { "ADC Left", NULL, "ADC PGA Left" }, >> + { "ADC Right Sel", "Right", "ADC Right" }, >> + { "ADC Right Sel", "Left", "ADC Left" }, >> + { "ADC Left Sel", "Right", "ADC Right" }, >> + { "ADC Left Sel", "Left", "ADC Left" }, >> + { "ADC OUT Right", NULL, "ADC Right Sel" }, >> + { "ADC OUT Left", NULL, "ADC Left Sel" }, >> + { "Capture", NULL, "ADC OUT Right" }, >> + { "Capture", NULL, "ADC OUT Left" }, >> +}; >> + >> static int t9015_set_bias_level(struct snd_soc_component *component, >> enum snd_soc_bias_level level) >> { >> @@ -241,6 +465,18 @@ static int t9015_component_probe(struct snd_soc_component *component) >> return 0; >> } >> >> +static int a1_t9015_component_probe(struct snd_soc_component *component) >> +{ >> + /* >> + * This configuration was stealed from original Amlogic's driver to >> + * reproduce the behavior of the driver more accurately. However, it is >> + * not known for certain what it actually affects. >> + */ >> + snd_soc_component_write(component, POWER_CFG, 0x00010000); >> + >> + return 0; >> +} >> + >> static const struct snd_soc_component_driver t9015_codec_driver = { >> .probe = t9015_component_probe, >> .set_bias_level = t9015_set_bias_level, >> @@ -254,6 +490,19 @@ static const struct snd_soc_component_driver t9015_codec_driver = { >> .endianness = 1, >> }; >> >> +static const struct snd_soc_component_driver a1_t9015_codec_driver = { >> + .probe = a1_t9015_component_probe, >> + .set_bias_level = t9015_set_bias_level, >> + .controls = a1_t9015_snd_controls, >> + .num_controls = ARRAY_SIZE(a1_t9015_snd_controls), >> + .dapm_widgets = a1_t9015_dapm_widgets, >> + .num_dapm_widgets = ARRAY_SIZE(a1_t9015_dapm_widgets), >> + .dapm_routes = a1_t9015_dapm_routes, >> + .num_dapm_routes = ARRAY_SIZE(a1_t9015_dapm_routes), >> + .suspend_bias_off = 1, >> + .endianness = 1, >> +}; >> + >> static int t9015_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -315,11 +564,21 @@ static const struct t9015_match_data t9015_match_data = { >> .max_register = POWER_CFG, >> }; >> >> +static const struct t9015_match_data a1_t9015_match_data = { >> + .component_drv = &a1_t9015_codec_driver, >> + .dai_drv = &a1_t9015_dai, >> + .max_register = LINEIN_CFG, >> +}; >> + >> static const struct of_device_id t9015_ids[] __maybe_unused = { >> { >> .compatible = "amlogic,t9015", >> .data = &t9015_match_data, >> }, >> + { >> + .compatible = "amlogic,t9015-a1", >> + .data = &a1_t9015_match_data, >> + }, >> { } >> }; >> MODULE_DEVICE_TABLE(of, t9015_ids); > > -- Best regards Jan Dakinevich
On Mon, 18 Mar 2024 at 23:23, Gabor Juhos <j4g8y7@gmail.com> wrote: > > 2024. 03. 18. 15:16 keltezéssel, Dmitry Baryshkov írta: > > ... > > >> +static const u8 ipq_pll_huayra_regs[PLL_OFF_MAX_REGS] = { > >> + [PLL_OFF_L_VAL] = 0x08, > >> + [PLL_OFF_ALPHA_VAL] = 0x10, > >> + [PLL_OFF_USER_CTL] = 0x18, > >> + [PLL_OFF_CONFIG_CTL] = 0x20, > >> + [PLL_OFF_CONFIG_CTL_U] = 0x24, > >> + [PLL_OFF_STATUS] = 0x28, > >> + [PLL_OFF_TEST_CTL] = 0x30, > >> + [PLL_OFF_TEST_CTL_U] = 0x34, > >> }; > > > > Can you please move this to clk_alpha_pll? We can then drop it from > > clk-cbf-8996.c too. > > Sure, I can do that. By any chance, do you have a suggestion for the name of the > new enum value to be used in the clk_alpha_pll_regs array? > > CLK_ALPHA_PLL_TYPE_HUAYRA_IPQ seems too generic, and it would be a bit > misleading using that for MSM8996 CBF. > > CLK_ALPHA_PLL_TYPE_HUAYRA_IPQ6018_A53 is quite long and it is also misleading. > > Maybe we could use CLK_ALPHA_PLL_TYPE_IPQ6018_A53 which is short and unique > enough and we could add an alias for that like CLK_ALPHA_PLL_TYPE_MSM8996_CBF or > something similar. HUAYRA_APSS ? > > Regards, > Gabor > -- With best wishes Dmitry
[-- Attachment #1: Type: text/plain, Size: 2584 bytes --] On Mon, Mar 18, 2024 at 05:20:50PM +0100, Krzysztof Kozlowski wrote: > On 17/03/2024 16:49, Conor Dooley wrote: > > On Sun, Mar 17, 2024 at 04:26:55PM +0100, Krzysztof Kozlowski wrote: > >> On 17/03/2024 16:23, Conor Dooley wrote: > >>> On Tue, Mar 12, 2024 at 07:50:35PM +0100, Krzysztof Kozlowski wrote: > >>>> Convert Samsung S3C6400/S3C6410 SoC clock controller bindings to DT > >>>> schema. > >>> > >>>> +description: | > >>>> + There are several clocks that are generated outside the SoC. It is expected > >>>> + that they are defined using standard clock bindings with following > >>>> + clock-output-names: > >>>> + - "fin_pll" - PLL input clock (xtal/extclk) - required, > >>>> + - "xusbxti" - USB xtal - required, > >>>> + - "iiscdclk0" - I2S0 codec clock - optional, > >>>> + - "iiscdclk1" - I2S1 codec clock - optional, > >>>> + - "iiscdclk2" - I2S2 codec clock - optional, > >>>> + - "pcmcdclk0" - PCM0 codec clock - optional, > >>>> + - "pcmcdclk1" - PCM1 codec clock - optional, only S3C6410. > >>> > >>> I know you've only transfered this from the text binding, but what is > >>> the relevance of this to the binding for this clock controller? This > >>> seems to be describing some ?fixed? clocks that must be provided in > >>> addition to this controller. I guess there's probably no other suitable > >>> place to mention these? > >> > >> To make it correct, these should be made clock inputs to the clock > >> controller, even if the driver does not take them, however that's > >> obsolete platform which might be removed from kernel this or next year, > >> so I don't want to spend time on it. > > > > I think the comment should probably mention that these are the expected > > inputs, part of me thought that that was what you were getting at but I > > wasn't sure if instead they were inputs to some other IP on the SoC. > > I can change it, but just to emphasize: in half a year or next year we > will probably remove entire platform, thus also this binding. I know, I saw that. I don't really care what you do given the platform is being deleted and it is unlikely that anyone is actually going to be assembling a from-scratch dtsi for this SoC. On the other hand, if you're doing a conversion, even in this scenario, I think it should be clear. I didn't ack the patch cos I figured you were taking the patch via the samsung tree (and on to Stephen) yourself, but here: Acked-by: Conor Dooley <conor.dooley@microchip.com> I'd rather argue about the definition of erratum instead of this :) [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --]
> Subject: [PATCH] clk: imx: imx8mp: Add pm_runtime support for power > saving > > Add pm_runtime support for power saving. In pm runtime suspend state the > registers will be reseted, so add registers save in pm runtime suspend and > restore them in pm runtime resume. > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> Reviewed-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/clk/imx/clk-imx8mp-audiomix.c | 99 ++++++++++++++++++++++++++- > 1 file changed, 96 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk- > imx8mp-audiomix.c > index 55ed211a5e0b..d2bf53e2aacf 100644 > --- a/drivers/clk/imx/clk-imx8mp-audiomix.c > +++ b/drivers/clk/imx/clk-imx8mp-audiomix.c > @@ -7,10 +7,12 @@ > > #include <linux/clk-provider.h> > #include <linux/device.h> > +#include <linux/io.h> > #include <linux/mod_devicetable.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > > #include <dt-bindings/clock/imx8mp-clock.h> > > @@ -18,6 +20,7 @@ > > #define CLKEN0 0x000 > #define CLKEN1 0x004 > +#define EARC 0x200 > #define SAI1_MCLK_SEL 0x300 > #define SAI2_MCLK_SEL 0x304 > #define SAI3_MCLK_SEL 0x308 > @@ -26,6 +29,12 @@ > #define SAI7_MCLK_SEL 0x314 > #define PDM_SEL 0x318 > #define SAI_PLL_GNRL_CTL 0x400 > +#define SAI_PLL_FDIVL_CTL0 0x404 > +#define SAI_PLL_FDIVL_CTL1 0x408 > +#define SAI_PLL_SSCG_CTL 0x40C > +#define SAI_PLL_MNIT_CTL 0x410 > +#define IPG_LP_CTRL 0x504 > +#define REGS_NUM 16 > > #define SAIn_MCLK1_PARENT(n) \ > static const struct clk_parent_data \ > @@ -182,13 +191,65 @@ static struct clk_imx8mp_audiomix_sel sels[] = { > CLK_SAIn(7) > }; > > +struct clk_imx8mp_audiomix_regs { > + u32 regs_num; > + u32 regs_off[]; > +}; > + > +static const struct clk_imx8mp_audiomix_regs audiomix_regs = { > + .regs_num = REGS_NUM, > + .regs_off = { > + CLKEN0, > + CLKEN1, > + EARC, > + SAI1_MCLK_SEL, > + SAI2_MCLK_SEL, > + SAI3_MCLK_SEL, > + SAI5_MCLK_SEL, > + SAI6_MCLK_SEL, > + SAI7_MCLK_SEL, > + PDM_SEL, > + SAI_PLL_GNRL_CTL, > + SAI_PLL_FDIVL_CTL0, > + SAI_PLL_FDIVL_CTL1, > + SAI_PLL_SSCG_CTL, > + SAI_PLL_MNIT_CTL, > + IPG_LP_CTRL > + }, > +}; > + > +struct clk_imx8mp_audiomix_drvdata { > + void __iomem *base; > + u32 regs_save[REGS_NUM]; > +}; > + > +static void clk_imx8mp_audiomix_save_restore(struct device *dev, bool > +save) { > + struct clk_imx8mp_audiomix_drvdata *drvdata = > dev_get_drvdata(dev); > + void __iomem *base = drvdata->base; > + int i; > + > + if (save) { > + for (i = 0; i < audiomix_regs.regs_num; i++) > + drvdata->regs_save[i] = readl(base + > audiomix_regs.regs_off[i]); > + } else { > + for (i = 0; i < audiomix_regs.regs_num; i++) > + writel(drvdata->regs_save[i], base + > audiomix_regs.regs_off[i]); > + } > +} > + > static int clk_imx8mp_audiomix_probe(struct platform_device *pdev) { > + struct clk_imx8mp_audiomix_drvdata *drvdata; > struct clk_hw_onecell_data *priv; > struct device *dev = &pdev->dev; > void __iomem *base; > struct clk_hw *hw; > - int i; > + int i, ret; > + > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > + if (!drvdata) > + return -ENOMEM; > > priv = devm_kzalloc(dev, > struct_size(priv, hws, > IMX8MP_CLK_AUDIOMIX_END), @@ -202,6 +263,9 @@ static int > clk_imx8mp_audiomix_probe(struct platform_device *pdev) > if (IS_ERR(base)) > return PTR_ERR(base); > > + drvdata->base = base; > + dev_set_drvdata(dev, drvdata); > + > for (i = 0; i < ARRAY_SIZE(sels); i++) { > if (sels[i].num_parents == 1) { > hw = devm_clk_hw_register_gate_parent_data(dev, > @@ -257,10 +321,38 @@ static int clk_imx8mp_audiomix_probe(struct > platform_device *pdev) > if (IS_ERR(hw)) > return PTR_ERR(hw); > > - return devm_of_clk_add_hw_provider(&pdev->dev, > of_clk_hw_onecell_get, > - priv); > + ret = devm_of_clk_add_hw_provider(&pdev->dev, > of_clk_hw_onecell_get, > + priv); > + if (ret) > + return ret; > + > + pm_runtime_enable(&pdev->dev); > + clk_imx8mp_audiomix_save_restore(&pdev->dev, true); > + > + return 0; > } > > +static int clk_imx8mp_audiomix_runtime_suspend(struct device *dev) { > + clk_imx8mp_audiomix_save_restore(dev, true); > + > + return 0; > +} > + > +static int clk_imx8mp_audiomix_runtime_resume(struct device *dev) { > + clk_imx8mp_audiomix_save_restore(dev, false); > + > + return 0; > +} > + > +static const struct dev_pm_ops clk_imx8mp_audiomix_pm_ops = { > + SET_RUNTIME_PM_OPS(clk_imx8mp_audiomix_runtime_suspend, > + clk_imx8mp_audiomix_runtime_resume, NULL) > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) > +}; > + > static const struct of_device_id clk_imx8mp_audiomix_of_match[] = { > { .compatible = "fsl,imx8mp-audio-blk-ctrl" }, > { /* sentinel */ } > @@ -272,6 +364,7 @@ static struct platform_driver > clk_imx8mp_audiomix_driver = { > .driver = { > .name = "imx8mp-audio-blk-ctrl", > .of_match_table = clk_imx8mp_audiomix_of_match, > + .pm = &clk_imx8mp_audiomix_pm_ops, > }, > }; > > -- > 2.34.1
> Subject: Re: [PATCH v4 0/6] Add support i.MX95 BLK CTL module clock > features > > On 24-03-18, Peng Fan wrote: > > > Subject: Re: [PATCH v4 0/6] Add support i.MX95 BLK CTL module clock > > > features > > > > > > On 24-03-18, Peng Fan wrote: > > > > Hi Marco, > > > > > > > > > Subject: Re: [PATCH v4 0/6] Add support i.MX95 BLK CTL module > > > > > clock features > > > > > > > > > > Hi Peng, > > > > > > > > > > thank for the patchset. > > > > > > > > > > On 24-03-14, Peng Fan (OSS) wrote: > > > > > > i.MX95's several MIXes has BLK CTL module which could be used > > > > > > for clk settings, QoS settings, Misc settings for a MIX. This > > > > > > patchset is to add the clk feature support, including > > > > > > dt-bindings > > > > > > > > > > I have to ask since there is almost no public documentation > > > > > available yet. The > > > > > i.MX95 does have an system-controller for managing pinmux > > > > > settings and power-domains, right? > > > > > > > > Yes. > > > > > > > > If this is the case, why not making use of it via the > > > > > standard scmi_pm_domain.c driver? > > > > > > > > The SCMI firmware not handle the BLK CTL stuff, but blk ctl stuff > > > > is a mix of clk, qos, module specific things. It is not good for > > > > SCMI firmare to handle it. > > > > > > Currently most of the blk-ctrl users do use the blk-ctrl as > > > power-domain consumer, except for the isi and the audio part. > > > > Yes, for i.MX8M. > > > > > So as I said above the > > > scmi_pm_domain.c should be able to supply this. The audio blk-ctrl > > > could be abstracted via the clk-scmi.c driver. The ISI is another topic. > > > > Pengutronix rejected the efforts for putting blk ctrl stuff in ATF for > > i.MX8M before. So we take the kernel power domain approach to handle > > blk ctrl clk gating. > > AFAIK the problem here was that your proposal had an layering violation > even worse it was an layering inversion since the TF-A (lower layer) code > updated CCM registers which were under control of Linux (upper layer). > > > > What you're are going to do here is to put pinctrl etc into SCMI > > > firmware and power-control into Linux, which sound to me like an > > > 50/50 approach and IMHO is rather sub-optimal. > > > > Now back to i.MX95 which supports function safety. > > > > The SCMI firmware only handle CCM/SRC/GPC for clock/power, it not > > handle blk ctrl. > > I know that. > > > The BLK CTRL registers are not only for clk gating, it has other > > module specific functions. > > I suspected that this is the case for i.MX95 too :/ Yes, BLK CTRL is a group of control registers for a MIX, including i.MX95. > > > Moving BLK CTRL to SCMI firmware, means linux accessing module > > specific functions needs go through vendor SCMI protocol. > > Right and here it's a bit complicated to have an proper interface. > Therefore I'm not again the solution of keeping the blk-ctrl within Linux. > > > And BLK CTRL stuff is MIX level stuff, it is not SOC level stuff as > > CCM which is system critical resource. > > Hm.. it's still SoC level albeit the area is more limited to specific functions like > HSIO, MEDIA, GPU, ... > > > (BLK CTLR, I mean non system critical BLK CTRL, such as GPU,VPU,DISP) > > What's system critical is up to the system design but I get your point that > having an safe DISP stack is not going to happen. Right. There is a M7 core runs safety application, there is a system controller core manage clk/power/pin and etc. The DISP related BLK CTRL could be directly assigned to M7 core if there is such usecase. > > > The other approach is to let ATF as SCMI server to handle BLK CTRL > > stuff, But I not see benefits. > > How is that any different from putting it into the system-controller firmware. > > > > To quote your online available fact sheet: > > > > > > 8<---------------------------------------------------------- > > > ENERGY FLEX ARCHITECTURE > > > > > > The i,MX 95 family is designed to be configurable and scalable, with > > > multiple heterogenous processing domains. > > > This includes an application domain with up to 6 Arm Cortex A55 > > > cores, a high-performance real-time domain with Arm Cortex M7, and > > > low- power/safety domain with Arm Cortex M33, each able to access > > > interfaces including CAN-FD, 10GbE networking, PCIe Gen 3 x1 > > > interfaces, and accelerators such as V2X, ISP, and VPU. > > > 8<---------------------------------------------------------- > > > > > > 8<---------------------------------------------------------- > > > HIGH-PERFORMANCE COMPUTE > > > The i.MX 95 family capabilities include a multi-core application > > > domain with up to six Arm Cortex(r)-A55 cores, as well as two > > > independent real-time domains for safety/low-power, and > > > high-performance real-time use, consisting of high-performance Arm > > > Cortex-M7 and Arm Cortex-M33 CPUs, combining low-power, real-time, > > > and high-performance processing. The i.MX > > > 95 family is designed to enable ISO 26262 ASIL-B and SIL-2 IEC 61508 > > > compliant platforms, with the safety domain serving as a critical > > > capability for many automotive and industrial applications. > > > ... > > > 8<---------------------------------------------------------- > > > > > > To me this sound like we can turn of the power/clock of an hardware > > > block which was assigned to a core running SIL-2 certified software > > > from an non- critical core running Linux if we follow that approach. > > > Also the > > > SIL-2 software requires the non-critical software to turn on the > > > power of these hardware blocks. Is this correct? > > > > Non-critical software not able to turn off power/clock of a critical > > resource in safety software domain. > > Safety software not require non-safety software to turn on power/clocks. > > Due to lack of documentation I don't know how you implemented this in > HW/SW, also the system-design is telling us which parts should be seen as > safe and which don't. However I get your point, VPUMIX is not going to be a > part of the safe partition albeit it "could" due to complexity. If safe function needs VPU feature, VPUMIX could be totally assigned to M7 core through TRDC isolation, not assigned its BLK CTRL to system controller core. > > > CCM/SRC/GPC is handled by SCMI firmware, agent w/o safety needs use > > SCMI API to request SCMI firmware to enable clock/power for a module. > > The SCMI firmware will check whether the agent is allowed to touch a > > clock entry or a power entry. > > I got this. I still don't like the 50/50 approach but I also get your point about > the GPR registers which is the only valid argument to me of not putting the > blk-ctrl handling into the system-controller firmware. Thanks, Peng. > > Regards, > Marco
Hi Biju, Thank you for the patch. On Mon, Mar 18, 2024 at 11:08:41AM +0000, Biju Das wrote: > The clk_disable_unprepare() doesn't guarantee that a clock is gated after > the execution as it is driver dependent. The Renesas and most of the other > platforms don't wait until clock is stopped because of performance reason. Is that really the case for most platforms ? I've never seen that being an issue in practice. > But these platforms wait while turning on the clock. > > The normal case for shutting down the clock is unbind/close/suspend or > error paths in the driver. Not waiting for the shutting down the clock > will improve the suspend time. > > But on RZ/G2L Camera Data Receiving Unit (CRU) IP, initially the vclk is > on. Before enabling link reception, we need to wait for vclk to be off > and after enabling reception, we need to turn the vlck on. Special cases > like this requires a sync API for clock gating. > > Add clk_poll_disable_unprepare() to poll the clock gate operation that > guarantees gating of clk after the execution. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * Added WARN_ON(enable count non-zero) and return an error code (-EBUSY), > if the user is not the sole user of the clock and the enable count is > non-zero. > * Returned an error if there's no is_enabled() callback. > RFC->v2: > * Renamed clk_disable_unprepare_sync()-->clk_poll_disable_unprepare() > * Redesigned to make use of __clk_is_enabled() to poll the clock gating. > --- > drivers/clk/clk.c | 29 ++++++++++++++++++++++++++++ > include/linux/clk.h | 46 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 75 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index f5fa91a339d7..e10bb14c904d 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -13,6 +13,7 @@ > #include <linux/mutex.h> > #include <linux/spinlock.h> > #include <linux/err.h> > +#include <linux/iopoll.h> > #include <linux/list.h> > #include <linux/slab.h> > #include <linux/of.h> > @@ -1160,6 +1161,34 @@ void clk_disable(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_disable); > > +/** > + * clk_poll_disabled - poll for clock gating. > + * @clk: the clk that is going to stop > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * It polls for a clk to be stopped. > + */ > +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us) > +{ > + bool status; > + > + if (IS_ERR_OR_NULL(clk)) > + return 0; > + > + if (!clk->core->ops->is_enabled) > + return -EOPNOTSUPP; > + > + if (WARN(__clk_get_enable_count(clk), "clk is in use\n")) > + return -EBUSY; A WARN() is fairly bad, given how easy it could be to this this condition. To make it safe in the genral case for drivers to use this API, we may need a way to acquire a clock exclusively, which would complexify the API quite a bit. > + > + return read_poll_timeout(__clk_is_enabled, status, !status, sleep_us, > + timeout_us, false, clk); > +} > +EXPORT_SYMBOL_GPL(clk_poll_disabled); > + > static int clk_core_enable(struct clk_core *core) > { > int ret = 0; > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 84b02518791f..7f714ecce0eb 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -693,6 +693,20 @@ int __must_check clk_bulk_enable(int num_clks, > */ > void clk_disable(struct clk *clk); > > +/** > + * clk_poll_disabled - inform the system whether the clock source is stopped. > + * @clk: clock source > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * Poll for clock gating and Inform the system about it's status. > + * > + * Context: May sleep. > + */ > +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us); > + > /** > * clk_bulk_disable - inform the system when the set of clks is no > * longer required. > @@ -1030,6 +1044,11 @@ static inline int __must_check clk_bulk_enable(int num_clks, > > static inline void clk_disable(struct clk *clk) {} > > +static inline int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, > + u64 timeout_us) > +{ > + return 0; > +} > > static inline void clk_bulk_disable(int num_clks, > const struct clk_bulk_data *clks) {} > @@ -1121,6 +1140,33 @@ static inline void clk_disable_unprepare(struct clk *clk) > clk_unprepare(clk); > } > > +/** > + * clk_poll_disable_unprepare - Poll clk_disable_unprepare > + * @clk: clock source > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * Context: May sleep. > + * > + * This function polls until the clock has stopped. > + * > + * Returns success (0) or negative errno. > + */ > +static inline int clk_poll_disable_unprepare(struct clk *clk, > + unsigned long sleep_us, > + u64 timeout_us) > +{ > + int ret; > + > + clk_disable(clk); > + ret = clk_poll_disabled(clk, sleep_us, timeout_us); > + clk_unprepare(clk); What happens in the clk_disable_unprepare() case, if the clk_unprepare() function is called on a clock that hasn't been synchronously disabled ? This is ill-defined, a clock provider driver that implements .disable() asynchronously would see its .unprepare() operation called with different clock states. That behaviour is error-prone, especially given that it could be difficult to test clock provider drivers to ensure that handle both cases correctly. One option could be to turn the .unprepare() operation into a synchronization point, requiring drivers that implement .disable() asynchronously to implement synchronization in .unprepare(). That way you wouldn't need a new API function for clock consumers. The downside is that consumers that call clk_disable_unprepare() will never benefit from the .disable() operation being asynchronous, which may defeat the whole point of this exercise. I'm starting to wonder if the simplest option in your case wouldn't be to make your clock provider synchronous for the vclk... > + > + return ret; > +} > + > static inline int __must_check > clk_bulk_prepare_enable(int num_clks, const struct clk_bulk_data *clks) > { -- Regards, Laurent Pinchart
On 3/18/24 16:48, Mark Brown wrote:
> On Sun, Mar 17, 2024 at 07:27:14PM +0300, Jan Dakinevich wrote:
>
>> Both mic bias and ADC's input mode depends on schematics and should be
>> configurable. What is the better way to give access to these parameters?
>> Device tree?
>
> Yes.
>
>>>> + SOC_SINGLE("ADC Mic Bias Switch", LINEIN_CFG, MICBIAS_EN, 1, 0),
>>>> + SOC_ENUM("ADC Mic Bias Level", a1_adc_mic_bias_level),
>
>>> Why would micbias be user controlled rather than a DAPM widget as
>>> normal?
>
>> Yes, I could use SND_SOC_DAPM_SUPPLY, but it supports only raw values,
>> and doesn't supports enums. Here, I want to use enum to restrict
>> possible values, because only these values mentioned in the
>> documentation that I have.
>
> A supply is an on/off switch not an enum. Users should not be selecting
> values at all.
Ok. For me it is great if I am free to move these kcontrols to device tree.
--
Best regards
Jan Dakinevich
Hi Biju, Thank you for the patch. On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote: > > The API's related to clk disable operation does not explicitly > states the synchoronous or asynchrous behaviour as it is driver s/synchoronous/synchronous/ > dependent. So make this part clear in API documentation. You need to explain the rationale here, why asynchronous behaviour is preferred. > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * No change. > v2: > * New patch. > --- > drivers/clk/clk.c | 3 ++- > include/linux/clk-provider.h | 3 ++- > include/linux/clk.h | 3 ++- > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 25371c91a58f..f5fa91a339d7 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core) > * if the operation may sleep. One example is a clk which is accessed over > * I2c. In the complex case a clk gate operation may require a fast and a slow > * part. It is this reason that clk_unprepare and clk_disable are not mutually > - * exclusive. In fact clk_disable must be called before clk_unprepare. > + * exclusive. In fact clk_disable must be called before clk_unprepare. The > + * synchronous or asynchronous clock gating operation is driver dependent. If synchronous operation is not guaranteed, then it's asynchonous. Asynchronous doesn't mean slow, even an asynchronous provider can complete the disable operation before the function returns to the caller. All it means is that there's no guarantee of synchronous operation. I would document it as such: * This function is asynchronous, if may return before the clock provider * completes the unprepare operation. However, below you're addressing the disable operation. Did you mean to patch the documentation for clk_prepare() instead ? Making clk_unprepare() asynchronous seems a bit weird, given that the function may sleep and is expected to take more time. > */ > void clk_unprepare(struct clk *clk) > { > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 4a537260f655..5b493024e1ec 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -113,7 +113,8 @@ struct clk_duty { > * sleep. > * > * @disable: Disable the clock atomically. Called with enable_lock held. > - * This function must not sleep. > + * This function must not sleep. The synchronous or asynchronous > + * disabling of the clock is driver dependent. As this is the documentation that targets clock providers, I would expand it and explain why a provider may want to make the disable operation not synchronous. > * > * @is_enabled: Queries the hardware to determine if the clock is enabled. > * This function must not sleep. Optional, if this op is not .is_enabled() should become mandatory if .disable() is not synchronous. The relationship between the two operations should be better explained. > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 00623f4de5e1..84b02518791f 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks, > * @clk: clock source > * > * Inform the system that a clock source is no longer required by > - * a driver and may be shut down. > + * a driver and may be shut down. It is not guaranteed to ever actually > + * be stopped, that will be driver dependent. This is the documentation of clk_bulk_disable(), you should address clk_disable() too. I've just noticed that both functions are documented in two places, in include/linux/clk.h, and in drivers/clk/. I wonder why that is. It sounds like it should be fixed, or you'll have to patch both documentation blocks. There's another issue that I'll raise in the review of 2/3. > * > * May be called from atomic contexts. > * -- Regards, Laurent Pinchart
On 3/18/24 13:17, Jerome Brunet wrote: > > On Sun 17 Mar 2024 at 17:17, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > >> On 3/15/24 11:58, Jerome Brunet wrote: >>> >>> On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: >>> >>>> Existing values were insufficient to produce accurate clock for audio >>>> devices. New values are safe and most suitable to produce 48000Hz sample >>>> rate. >>> >>> The hifi pll is not about 48k only. I see no reason to restrict the PLL >>> to a single setting. >>>> You've provided no justification why the PLL driver can't reach the same >>> setting for 48k. The setting below is just the crude part. the fine >>> tuning is done done with the frac parameter so I doubt this provides a >>> more accurate rate. >>> >> >> You are right, it is not about 48k only. However, there are two issues. >> >> First, indeed, I could just extend the range of multipliers to 1..255. > > Why 1..255 ? This is not what I'm pointing out > > According to the datasheet - the range is 32 - 64, as currently > set in the driver. > Could you point where in the doc the range 32..64 is documented? Documentation that I have may be not so complete, but I don't see there any mention about it. Anyway, range 32..64 of multipliers is not enough to produce accurate clock, and a need 128 for 48kHz. > The change you have provided request a multipler of 128/5 = 25,6 > If you put assigned-rate = 614400000 in DT, I see no reason can find the > same solution on its own. > The reasoning is following. I don't know why 32..64 range was declared for this clock, and whether it would be safe to extend it and include 128, which is required for 48kHz. But I know, that multiplier=128 is safe and works fine (together divider=5). >> But I am unsure if hifi_pll is able to handle whole range of >> mulptipliers. The value 128 is taken from Amlogic's branch, and I am >> pretty sure that it works. > >> >> Second, unfortunately frac parameter currently doesn't work. When frac >> is used enabling of hifi_pll fails in meson_clk_pll_wait_lock(). I see >> it when try to use 44100Hz and multipliers' range is set to 1..255. So, >> support of other rates than 48k requires extra effort. > > Then your change is even more problematic because it certainly does not > disable frac ... which you say is broken. > > That parameter should be removed with a proper comment explaining why > you are disabling it. That type a limitation / known issue should be > mentionned in your change. > Handling of frac should not be removed, it should be fixed to achieve another rates. But that is not the goal of this commit. >> >>>> >>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>> --- >>>> drivers/clk/meson/a1-pll.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c >>>> index 4325e8a6a3ef..00e06d03445b 100644 >>>> --- a/drivers/clk/meson/a1-pll.c >>>> +++ b/drivers/clk/meson/a1-pll.c >>>> @@ -74,9 +74,9 @@ static struct clk_regmap fixed_pll = { >>>> }, >>>> }; >>>> >>>> -static const struct pll_mult_range hifi_pll_mult_range = { >>>> - .min = 32, >>>> - .max = 64, >>>> +static const struct pll_params_table hifi_pll_params_table[] = { >>>> + PLL_PARAMS(128, 5), >>>> + { }, >>>> }; >>>> >>>> static const struct reg_sequence hifi_init_regs[] = { >>>> @@ -124,7 +124,7 @@ static struct clk_regmap hifi_pll = { >>>> .shift = 6, >>>> .width = 1, >>>> }, >>>> - .range = &hifi_pll_mult_range, >>>> + .table = hifi_pll_params_table, >>>> .init_regs = hifi_init_regs, >>>> .init_count = ARRAY_SIZE(hifi_init_regs), >>>> }, >>> >>> > > -- Best regards Jan Dakinevich
2024. 03. 18. 15:16 keltezéssel, Dmitry Baryshkov írta:
...
>> +static const u8 ipq_pll_huayra_regs[PLL_OFF_MAX_REGS] = {
>> + [PLL_OFF_L_VAL] = 0x08,
>> + [PLL_OFF_ALPHA_VAL] = 0x10,
>> + [PLL_OFF_USER_CTL] = 0x18,
>> + [PLL_OFF_CONFIG_CTL] = 0x20,
>> + [PLL_OFF_CONFIG_CTL_U] = 0x24,
>> + [PLL_OFF_STATUS] = 0x28,
>> + [PLL_OFF_TEST_CTL] = 0x30,
>> + [PLL_OFF_TEST_CTL_U] = 0x34,
>> };
>
> Can you please move this to clk_alpha_pll? We can then drop it from
> clk-cbf-8996.c too.
Sure, I can do that. By any chance, do you have a suggestion for the name of the
new enum value to be used in the clk_alpha_pll_regs array?
CLK_ALPHA_PLL_TYPE_HUAYRA_IPQ seems too generic, and it would be a bit
misleading using that for MSM8996 CBF.
CLK_ALPHA_PLL_TYPE_HUAYRA_IPQ6018_A53 is quite long and it is also misleading.
Maybe we could use CLK_ALPHA_PLL_TYPE_IPQ6018_A53 which is short and unique
enough and we could add an alias for that like CLK_ALPHA_PLL_TYPE_MSM8996_CBF or
something similar.
Regards,
Gabor
On Fri, Mar 15, 2024 at 11:27:48PM +0100, Wadim Mueller wrote: > Add support for NXP S32 SoC family's GMAC to the stmmac network driver. This driver implementation is based on the patchset originally contributed by Chester Lin [1], which itself draws heavily from NXP's downstream implementation [2]. The patchset was never merged. Please wrap you commit message. > +#include <linux/device.h> > +#include <linux/ethtool.h> Is this one needed? > +static int s32_gmac_init(struct platform_device *pdev, void *priv) > +{ > + struct s32_priv_data *gmac = priv; > + u32 intf_sel; > + int ret; > + > + if (gmac->tx_clk) { > + ret = clk_prepare_enable(gmac->tx_clk); > + if (ret) { > + dev_err(&pdev->dev, "Can't set tx clock\n"); > + return ret; > + } > + } > + > + if (gmac->rx_clk) { > + ret = clk_prepare_enable(gmac->rx_clk); > + if (ret) { > + dev_err(&pdev->dev, "Can't set rx clock\n"); > + return ret; > + } > + } > + > + /* set interface mode */ > + if (gmac->ctrl_sts) { > + switch (gmac->intf_mode) { > + default: > + dev_info( > + &pdev->dev, > + "unsupported mode %u, set the default phy mode.\n", > + gmac->intf_mode); > + fallthrough; I would actually return -EINVAL. There is no backwards compatibility needed here, so force that the mode is always specified. > + case PHY_INTERFACE_MODE_SGMII: > + dev_info(&pdev->dev, "phy mode set to SGMII\n"); Please don't spam the kernel log. dev_dbg(). > +static void s32_fix_speed(void *priv, unsigned int speed, unsigned int mode) > +{ > + struct s32_priv_data *gmac = priv; > + > + if (!gmac->tx_clk || !gmac->rx_clk) > + return; > + > + /* SGMII mode doesn't support the clock reconfiguration */ > + if (gmac->intf_mode == PHY_INTERFACE_MODE_SGMII) > + return; > + > + switch (speed) { > + case SPEED_1000: > + dev_info(gmac->dev, "Set TX clock to 125M\n"); more dev_dbg(). A driver should generally be silent, unless something goes wrong. It is also questionable if dev_dbg() should be used. Once the driver actually works, you can throw away a lot of debug prints. Do you expect problems here in the future? > +static int s32_config_cache_coherency(struct platform_device *pdev, > + struct plat_stmmacenet_data *plat_dat) > +{ > + plat_dat->axi4_ace_ctrl = devm_kzalloc( > + &pdev->dev, sizeof(struct stmmac_axi4_ace_ctrl), GFP_KERNEL); > + > + if (!plat_dat->axi4_ace_ctrl) > + return -ENOMEM; > + > + plat_dat->axi4_ace_ctrl->tx_ar_reg = (ACE_CONTROL_SIGNALS << 16) | > + (ACE_CONTROL_SIGNALS << 8) | > + ACE_CONTROL_SIGNALS; > + > + plat_dat->axi4_ace_ctrl->rx_aw_reg = > + (ACE_CONTROL_SIGNALS << 24) | (ACE_CONTROL_SIGNALS << 16) | > + (ACE_CONTROL_SIGNALS << 8) | ACE_CONTROL_SIGNALS; > + > + plat_dat->axi4_ace_ctrl->txrx_awar_reg = > + (ACE_PROTECTION << 20) | (ACE_PROTECTION << 16) | > + (ACE_CONTROL_SIGNALS << 8) | ACE_CONTROL_SIGNALS; This looks like magic. Can the various shifts be replaced my #defines? Comments added? This makes changes in some of the core code. So it might be better to have a prerequisite patch adding cache coherency control, with a good commit message explaining it. > + > + return 0; > +} > + > +static int s32_dwmac_probe(struct platform_device *pdev) > +{ > + struct plat_stmmacenet_data *plat_dat; > + struct stmmac_resources stmmac_res; > + struct s32_priv_data *gmac; > + struct resource *res; > + const char *tx_clk, *rx_clk; > + int ret; > + > + ret = stmmac_get_platform_resources(pdev, &stmmac_res); > + if (ret) > + return ret; > + > + gmac = devm_kzalloc(&pdev->dev, sizeof(*gmac), GFP_KERNEL); > + if (!gmac) > + return PTR_ERR(gmac); > + > + gmac->dev = &pdev->dev; > + > + /* S32G control reg */ > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + gmac->ctrl_sts = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR_OR_NULL(gmac->ctrl_sts)) { > + dev_err(&pdev->dev, "S32G config region is missing\n"); > + return PTR_ERR(gmac->ctrl_sts); > + } > + > + plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac); > + if (IS_ERR(plat_dat)) > + return PTR_ERR(plat_dat); > + > + plat_dat->bsp_priv = gmac; > + > + switch (plat_dat->phy_interface) { > + case PHY_INTERFACE_MODE_SGMII: > + tx_clk = "tx_sgmii"; > + rx_clk = "rx_sgmii"; > + break; > + case PHY_INTERFACE_MODE_RGMII: > + case PHY_INTERFACE_MODE_RGMII_ID: > + case PHY_INTERFACE_MODE_RGMII_TXID: > + case PHY_INTERFACE_MODE_RGMII_RXID: > + tx_clk = "tx_rgmii"; > + rx_clk = "rx_rgmii"; > + break; > + case PHY_INTERFACE_MODE_RMII: > + tx_clk = "tx_rmii"; > + rx_clk = "rx_rmii"; > + break; > + case PHY_INTERFACE_MODE_MII: > + tx_clk = "tx_mii"; > + rx_clk = "rx_mii"; > + break; > + default: > + dev_err(&pdev->dev, "Not supported phy interface mode: [%s]\n", > + phy_modes(plat_dat->phy_interface)); > + return -EINVAL; > + }; > + > + gmac->intf_mode = plat_dat->phy_interface; > + > + /* DMA cache coherency settings */ > + if (of_dma_is_coherent(pdev->dev.of_node)) { > + ret = s32_config_cache_coherency(pdev, plat_dat); > + if (ret) > + return ret; > + } > + > + /* tx clock */ > + gmac->tx_clk = devm_clk_get(&pdev->dev, tx_clk); > + if (IS_ERR(gmac->tx_clk)) { > + dev_info(&pdev->dev, "tx clock not found\n"); > + gmac->tx_clk = NULL; Is the clock really optional? I would also print the name of the clock which is missing. > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -324,6 +324,10 @@ static void stmmac_clk_csr_set(struct stmmac_priv *priv) > priv->clk_csr = STMMAC_CSR_150_250M; > else if ((clk_rate >= CSR_F_250M) && (clk_rate <= CSR_F_300M)) > priv->clk_csr = STMMAC_CSR_250_300M; > + else if ((clk_rate >= CSR_F_300M) && (clk_rate < CSR_F_500M)) > + priv->clk_csr = STMMAC_CSR_300_500M; > + else if ((clk_rate >= CSR_F_500M) && (clk_rate < CSR_F_800M)) > + priv->clk_csr = STMMAC_CSR_500_800M; Also seems like something which could be a patch of its own. Ideally you want lots of small patches which are obviously correct. Part of being obviously correct is the commit message, which is easier to write when the patch is small and only does one thing. Andrew
On 18.03.2024 11:25, Varshini.Rajendran@microchip.com wrote: > Hi Claudiu, > > On 11/03/24 11:28 am, claudiu beznea wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 23.02.2024 19:28, Varshini Rajendran wrote: >>> Add a driver for the PMC clocks of sam9x7 Soc family. >>> >>> Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> >>> --- >>> Changes in v4: >>> - Changed variable name alloc_mem to clk_mux_buffer to be more >>> suggestive >>> - Changed description of @f structure member appropriately >>> --- >>> drivers/clk/at91/Makefile | 1 + >>> drivers/clk/at91/sam9x7.c | 946 ++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 947 insertions(+) >>> create mode 100644 drivers/clk/at91/sam9x7.c >>> >>> diff --git a/drivers/clk/at91/Makefile b/drivers/clk/at91/Makefile >>> index 89061b85e7d2..8e3684ba2c74 100644 >>> --- a/drivers/clk/at91/Makefile >>> +++ b/drivers/clk/at91/Makefile [ ... ] >>> +static const struct { >>> + const char *n; >>> + const char *p; >>> + const struct clk_pll_layout *l; >>> + u8 t; >>> + const struct clk_pll_characteristics *c; >>> + unsigned long f; >>> + u8 eid; >>> +} sam9x7_plls[][PLL_ID_MAX] = { >>> + [PLL_ID_PLLA] = { >>> + { >>> + .n = "plla_fracck", >>> + .p = "mainck", >>> + .l = &plla_frac_layout, >>> + .t = PLL_TYPE_FRAC, >>> + /* >>> + * This feeds plla_divpmcck which feeds CPU. It should >>> + * not be disabled. >>> + */ >>> + .f = CLK_IS_CRITICAL | CLK_SET_RATE_GATE, >>> + .c = &plla_characteristics, >>> + }, >>> + >>> + { >>> + .n = "plla_divpmcck", >>> + .p = "plla_fracck", >>> + .l = &pll_divpmc_layout, >> >> You mentioned in "[PATCH v4 24/39] clk: at91: sam9x7: add support for HW >> PLL freq dividers" that this has div2 but it is registered w/ a layout that >> has .div2 = 0. > > This is handled in the above plla_fracck fractional part as defined in > the plla_frac_layout. Ah, right. I missed the changes in sam9x60_frac_pll_recalc_rate().
Hi Biju, On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote: > > The API's related to clk disable operation does not explicitly > states the synchoronous or asynchrous behaviour as it is driver > dependent. So make this part clear in API documentation. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * No change. > v2: > * New patch. > --- > drivers/clk/clk.c | 3 ++- > include/linux/clk-provider.h | 3 ++- > include/linux/clk.h | 3 ++- > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 25371c91a58f..f5fa91a339d7 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core) > * if the operation may sleep. One example is a clk which is accessed over > * I2c. In the complex case a clk gate operation may require a fast and a slow > * part. It is this reason that clk_unprepare and clk_disable are not mutually > - * exclusive. In fact clk_disable must be called before clk_unprepare. > + * exclusive. In fact clk_disable must be called before clk_unprepare. The > + * synchronous or asynchronous clock gating operation is driver dependent. > */ > void clk_unprepare(struct clk *clk) > { > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 4a537260f655..5b493024e1ec 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -113,7 +113,8 @@ struct clk_duty { > * sleep. > * > * @disable: Disable the clock atomically. Called with enable_lock held. > - * This function must not sleep. > + * This function must not sleep. The synchronous or asynchronous > + * disabling of the clock is driver dependent. s/driver\K/ and hardware/ Same in the first chunk actually. > * > * @is_enabled: Queries the hardware to determine if the clock is enabled. > * This function must not sleep. Optional, if this op is not > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 00623f4de5e1..84b02518791f 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks, > * @clk: clock source > * > * Inform the system that a clock source is no longer required by > - * a driver and may be shut down. > + * a driver and may be shut down. It is not guaranteed to ever actually > + * be stopped, that will be driver dependent. I'd rephrase this, taking other users into account: There's no guarantee that the clock stops within a particular time window or at all, depending on other users of the clock as well as the driver and hardware implementation. > * > * May be called from atomic contexts. > * -- Kind regards, Sakari Ailus
Hi Biju, Thanks for the update. On Mon, Mar 18, 2024 at 11:08:41AM +0000, Biju Das wrote: > The clk_disable_unprepare() doesn't guarantee that a clock is gated after > the execution as it is driver dependent. The Renesas and most of the other > platforms don't wait until clock is stopped because of performance reason. > But these platforms wait while turning on the clock. > > The normal case for shutting down the clock is unbind/close/suspend or > error paths in the driver. Not waiting for the shutting down the clock > will improve the suspend time. > > But on RZ/G2L Camera Data Receiving Unit (CRU) IP, initially the vclk is > on. Before enabling link reception, we need to wait for vclk to be off > and after enabling reception, we need to turn the vlck on. Special cases > like this requires a sync API for clock gating. > > Add clk_poll_disable_unprepare() to poll the clock gate operation that > guarantees gating of clk after the execution. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * Added WARN_ON(enable count non-zero) and return an error code (-EBUSY), > if the user is not the sole user of the clock and the enable count is > non-zero. > * Returned an error if there's no is_enabled() callback. > RFC->v2: > * Renamed clk_disable_unprepare_sync()-->clk_poll_disable_unprepare() > * Redesigned to make use of __clk_is_enabled() to poll the clock gating. > --- > drivers/clk/clk.c | 29 ++++++++++++++++++++++++++++ > include/linux/clk.h | 46 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 75 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index f5fa91a339d7..e10bb14c904d 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -13,6 +13,7 @@ > #include <linux/mutex.h> > #include <linux/spinlock.h> > #include <linux/err.h> > +#include <linux/iopoll.h> > #include <linux/list.h> > #include <linux/slab.h> > #include <linux/of.h> > @@ -1160,6 +1161,34 @@ void clk_disable(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_disable); > > +/** > + * clk_poll_disabled - poll for clock gating. > + * @clk: the clk that is going to stop > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * It polls for a clk to be stopped. > + */ We should have documentation either in the header or here, not both. I'd drop this. > +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us) > +{ > + bool status; > + > + if (IS_ERR_OR_NULL(clk)) > + return 0; > + > + if (!clk->core->ops->is_enabled) > + return -EOPNOTSUPP; > + > + if (WARN(__clk_get_enable_count(clk), "clk is in use\n")) > + return -EBUSY; > + > + return read_poll_timeout(__clk_is_enabled, status, !status, sleep_us, > + timeout_us, false, clk); > +} > +EXPORT_SYMBOL_GPL(clk_poll_disabled); > + > static int clk_core_enable(struct clk_core *core) > { > int ret = 0; > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 84b02518791f..7f714ecce0eb 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -693,6 +693,20 @@ int __must_check clk_bulk_enable(int num_clks, > */ > void clk_disable(struct clk *clk); > > +/** > + * clk_poll_disabled - inform the system whether the clock source is stopped. > + * @clk: clock source > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * Poll for clock gating and Inform the system about it's status. How about, instead: Poll for clock gating and return when either there's a timeout or the clock has been gated. Returns: 0 if the clock is successfully gated, error otherwise. Please run scripts/kerneldoc -Wall on this. > + * > + * Context: May sleep. > + */ > +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us); > + > /** > * clk_bulk_disable - inform the system when the set of clks is no > * longer required. > @@ -1030,6 +1044,11 @@ static inline int __must_check clk_bulk_enable(int num_clks, > > static inline void clk_disable(struct clk *clk) {} > > +static inline int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, > + u64 timeout_us) > +{ > + return 0; > +} > > static inline void clk_bulk_disable(int num_clks, > const struct clk_bulk_data *clks) {} > @@ -1121,6 +1140,33 @@ static inline void clk_disable_unprepare(struct clk *clk) > clk_unprepare(clk); > } > > +/** > + * clk_poll_disable_unprepare - Poll clk_disable_unprepare How about calling this clk_disable_sync_unprepare? I'm not sure if a special function is needed for something needed so rarely as you can already call clk_poll_disabled(). Maybe others have an opinion on this, too. > + * @clk: clock source > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * Context: May sleep. > + * > + * This function polls until the clock has stopped. > + * > + * Returns success (0) or negative errno. > + */ > +static inline int clk_poll_disable_unprepare(struct clk *clk, > + unsigned long sleep_us, > + u64 timeout_us) > +{ > + int ret; > + > + clk_disable(clk); > + ret = clk_poll_disabled(clk, sleep_us, timeout_us); > + clk_unprepare(clk); > + > + return ret; > +} > + > static inline int __must_check > clk_bulk_prepare_enable(int num_clks, const struct clk_bulk_data *clks) > { -- Kind regards, Sakari Ailus
Hi Claudiu, On Thu, Mar 7, 2024 at 3:07 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > RZ/G3S supports deep sleep states that it can reach with the help of the > TF-A. > > RZ/G3S has a few power domains (e.g. GIC) that need to be always-on while > Linux is running. These domains are initialized (and powered on) when > clock driver is probed. > > As the TF-A takes control at the very last(suspend)/first(resume) > phase of configuring the deep sleep state, it can do it's own settings on > power domains. > > Thus, to restore the proper Linux state, add rzg2l_cpg_resume() which > powers on the always-on domains and rzg2l_cpg_complete() which activates > the power down mode for the IPs selected through CPG_PWRDN_IP{1, 2}. > > Along with it, added the suspend_check member to the RZ/G2L power domain > data structure whose purpose is to checks if a domain can be powered off > while the system is going to suspend. This is necessary for the serial > console domain which needs to be powered on if no_console_suspend is > available in bootargs. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v2: > - none; this patch is new Thanks for your patch! > --- a/drivers/clk/renesas/rzg2l-cpg.c > +++ b/drivers/clk/renesas/rzg2l-cpg.c > @@ -1700,6 +1719,8 @@ static int __init rzg2l_cpg_pd_setup(struct rzg2l_cpg_pd *pd, bool always_on) > } else { > pd->genpd.power_on = rzg2l_cpg_power_on; > pd->genpd.power_off = rzg2l_cpg_power_off; > + if (flags & RZG2L_PD_F_CONSOLE) I think this should be replaced by some dynamic check, cfr. my comments on PATCH 9/10. > + pd->suspend_check = rzg2l_pd_suspend_check_console; > governor = &simple_qos_governor; > } > > @@ -1890,9 +1911,43 @@ static int __init rzg2l_cpg_probe(struct platform_device *pdev) > if (error) > return error; > > + dev_set_drvdata(dev, priv); > + > return 0; > } > > +static int rzg2l_cpg_resume(struct device *dev) > +{ > + struct rzg2l_cpg_priv *priv = dev_get_drvdata(dev); > + const struct rzg2l_cpg_info *info = priv->info; > + > + /* Power on always ON domains. */ > + for (unsigned int i = 0; i < info->num_pm_domains; i++) { > + if (info->pm_domains[i].flags & RZG2L_PD_F_ALWAYS_ON) { If you would check "priv-domains[i].flags & GENPD_FLAG_ALWAYS_ON" instead, I think you can make r9a08g045_pm_domains[] __initconst. You may need to make a copy of the name for pd->genpd.name, though. > + int ret = rzg2l_cpg_power_on(priv->domains[i]); I assume you are sure none of these domains are enabled by TF/A after system resume, or by the pmdomain core code? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 17/03/2024 16:49, Conor Dooley wrote:
> On Sun, Mar 17, 2024 at 04:26:55PM +0100, Krzysztof Kozlowski wrote:
>> On 17/03/2024 16:23, Conor Dooley wrote:
>>> On Tue, Mar 12, 2024 at 07:50:35PM +0100, Krzysztof Kozlowski wrote:
>>>> Convert Samsung S3C6400/S3C6410 SoC clock controller bindings to DT
>>>> schema.
>>>
>>>> +description: |
>>>> + There are several clocks that are generated outside the SoC. It is expected
>>>> + that they are defined using standard clock bindings with following
>>>> + clock-output-names:
>>>> + - "fin_pll" - PLL input clock (xtal/extclk) - required,
>>>> + - "xusbxti" - USB xtal - required,
>>>> + - "iiscdclk0" - I2S0 codec clock - optional,
>>>> + - "iiscdclk1" - I2S1 codec clock - optional,
>>>> + - "iiscdclk2" - I2S2 codec clock - optional,
>>>> + - "pcmcdclk0" - PCM0 codec clock - optional,
>>>> + - "pcmcdclk1" - PCM1 codec clock - optional, only S3C6410.
>>>
>>> I know you've only transfered this from the text binding, but what is
>>> the relevance of this to the binding for this clock controller? This
>>> seems to be describing some ?fixed? clocks that must be provided in
>>> addition to this controller. I guess there's probably no other suitable
>>> place to mention these?
>>
>> To make it correct, these should be made clock inputs to the clock
>> controller, even if the driver does not take them, however that's
>> obsolete platform which might be removed from kernel this or next year,
>> so I don't want to spend time on it.
>
> I think the comment should probably mention that these are the expected
> inputs, part of me thought that that was what you were getting at but I
> wasn't sure if instead they were inputs to some other IP on the SoC.
I can change it, but just to emphasize: in half a year or next year we
will probably remove entire platform, thus also this binding.
Best regards,
Krzysztof
On 18/03/2024 13:37, Peng Fan wrote:
>>> +
>>> +maintainers:
>>> + - Peng Fan <peng.fan@nxp.com>
>>> +
>>> +properties:
>>> + compatible:
>>> + items:
>>> + - const: nxp,imx95-display-csr
>>> + - const: syscon
>>
>> Why do you create five different bindings with almost the same contents?
>> Do you plan to grow on them, like add more compatibles here? Otherwise all
>> this could be in one binding.
>
> The blk ctrls are for different functions.
>
> We may expand the bindings to add more properties for the blk ctrls, but I
> am not sure as of now. I could merge them into one binding except
Bindings should be complete...
Best regards,
Krzysztof
On Mon, 18 Mar 2024 at 17:19, Nathan Chancellor <nathan@kernel.org> wrote:
>
> CONFIG_SM_GCC_8650 depends on ARM64 but it is selected by
> CONFIG_SM_GPUCC_8650, which can be selected on ARM, resulting in a
> Kconfig warning.
>
> WARNING: unmet direct dependencies detected for SM_GCC_8650
> Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=y] && (ARM64 || COMPILE_TEST [=n])
> Selected by [y]:
> - SM_GPUCC_8650 [=y] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=y]
>
> Add the same dependencies to CONFIG_SM_GPUCC_8650 to resolve the
> warning.
>
> Fixes: 8676fd4f3874 ("clk: qcom: add the SM8650 GPU Clock Controller driver")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> drivers/clk/qcom/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
On Mon, 18 Mar 2024 at 17:19, Nathan Chancellor <nathan@kernel.org> wrote:
>
> CONFIG_SC_GCC_8280XP depends on ARM64 but it is selected by
> CONFIG_SC_CAMCC_8280XP, which can be selected on ARM, resulting in a
> Kconfig warning.
>
> WARNING: unmet direct dependencies detected for SC_GCC_8280XP
> Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=y] && (ARM64 || COMPILE_TEST [=n])
> Selected by [y]:
> - SC_CAMCC_8280XP [=y] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=y]
>
> Add the same dependencies to CONFIG_SC_CAMCC_8280XP to resolve the
> warning.
>
> Fixes: ff93872a9c61 ("clk: qcom: camcc-sc8280xp: Add sc8280xp CAMCC")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> drivers/clk/qcom/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
CONFIG_SM_GCC_8650 depends on ARM64 but it is selected by CONFIG_SM_GPUCC_8650, which can be selected on ARM, resulting in a Kconfig warning. WARNING: unmet direct dependencies detected for SM_GCC_8650 Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=y] && (ARM64 || COMPILE_TEST [=n]) Selected by [y]: - SM_GPUCC_8650 [=y] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=y] Add the same dependencies to CONFIG_SM_GPUCC_8650 to resolve the warning. Fixes: 8676fd4f3874 ("clk: qcom: add the SM8650 GPU Clock Controller driver") Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- drivers/clk/qcom/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig index b9ff32cab329..1bb51a058872 100644 --- a/drivers/clk/qcom/Kconfig +++ b/drivers/clk/qcom/Kconfig @@ -1095,6 +1095,7 @@ config SM_GPUCC_8550 config SM_GPUCC_8650 tristate "SM8650 Graphics Clock Controller" + depends on ARM64 || COMPILE_TEST select SM_GCC_8650 help Support for the graphics clock controller on SM8650 devices. -- 2.44.0
CONFIG_SC_GCC_8280XP depends on ARM64 but it is selected by CONFIG_SC_CAMCC_8280XP, which can be selected on ARM, resulting in a Kconfig warning. WARNING: unmet direct dependencies detected for SC_GCC_8280XP Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=y] && (ARM64 || COMPILE_TEST [=n]) Selected by [y]: - SC_CAMCC_8280XP [=y] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=y] Add the same dependencies to CONFIG_SC_CAMCC_8280XP to resolve the warning. Fixes: ff93872a9c61 ("clk: qcom: camcc-sc8280xp: Add sc8280xp CAMCC") Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- drivers/clk/qcom/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig index 8ab08e7b5b6c..b9ff32cab329 100644 --- a/drivers/clk/qcom/Kconfig +++ b/drivers/clk/qcom/Kconfig @@ -474,6 +474,7 @@ config SC_CAMCC_7280 config SC_CAMCC_8280XP tristate "SC8280XP Camera Clock Controller" + depends on ARM64 || COMPILE_TEST select SC_GCC_8280XP help Support for the camera clock controller on Qualcomm Technologies, Inc -- 2.44.0
This series fixes two Kconfig warnings that I recently saw crop up in my build tests because the dependencies for newly added drivers in 6.8 are not correct. This is now the fourth set of changes to avoid warnings of this nature for the exact same reason... is there anything that can be done to ensure this does not continue to happen? See b6bcd1c0c27e ("clk: qcom: fix some Kconfig corner cases") 75d1d3a433f0 ("clk: qcom: Fix SM_GPUCC_8450 dependencies") e8d66d02defd ("clk: qcom: Fix SM_CAMCC_8550 dependencies") --- Nathan Chancellor (2): clk: qcom: Fix SC_CAMCC_8280XP dependencies clk: qcom: Fix SM_GPUCC_8650 dependencies drivers/clk/qcom/Kconfig | 2 ++ 1 file changed, 2 insertions(+) --- base-commit: 3066c521be9db14964d78c6c431c97a424468ded change-id: 20240318-fix-some-qcom-kconfig-deps-cc9be94f63ba Best regards, -- Nathan Chancellor <nathan@kernel.org>