| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <82159a29-3d34-4d02-00c5-2d25e8a8a118@amlogic.com>
Date: Thu, 4 May 2023 16:43:48 +0800
From: Yu Tu <yu.tu@...ogic.com>
To: Jerome Brunet <jbrunet@...libre.com>,
Dmitry Rokosov <ddrokosov@...rdevices.ru>
Cc: linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
Neil Armstrong <neil.armstrong@...aro.org>,
Kevin Hilman <khilman@...libre.com>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
kelvin.zhang@...ogic.com, qi.duan@...ogic.com
Subject: Re: [PATCH V7 4/4] clk: meson: s4: add support for Amlogic S4 SoC
peripheral clock controller
On 2023/5/2 1:37, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Thu 27 Apr 2023 at 16:15, Yu Tu <yu.tu@...ogic.com> wrote:
>
>> On 2023/4/26 19:05, Dmitry Rokosov wrote:
>>> [Some people who received this message don't often get email from
>>> ddrokosov@...rdevices.ru. Learn why this is important at
>>> https://aka.ms/LearnAboutSenderIdentification ]
>>> [ EXTERNAL EMAIL ]
>>> On Mon, Apr 17, 2023 at 02:50:05PM +0800, Yu Tu wrote:
>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>
>>>> Signed-off-by: Yu Tu <yu.tu@...ogic.com>
>>>> ---
>>>> drivers/clk/meson/Kconfig | 12 +
>>>> drivers/clk/meson/Makefile | 1 +
>>>> drivers/clk/meson/s4-peripherals.c | 3814 ++++++++++++++++++++++++++++
>>>> drivers/clk/meson/s4-peripherals.h | 217 ++
>>>> 4 files changed, 4044 insertions(+)
>>>> create mode 100644 drivers/clk/meson/s4-peripherals.c
>>>> create mode 100644 drivers/clk/meson/s4-peripherals.h
>>>>
>>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>>> index a663c90a3f3b..a6eb9fa15c74 100644
>>>> --- a/drivers/clk/meson/Kconfig
>>>> +++ b/drivers/clk/meson/Kconfig
>>>> @@ -128,4 +128,16 @@ config COMMON_CLK_S4_PLL
>>>> aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
>>>> Say Y if you want the board to work, because plls are the parent of most
>>>> peripherals.
>>>> +
>>>> +config COMMON_CLK_S4
>>>> + tristate "S4 SoC Peripherals clock controllers support"
>>>> + depends on ARM64
>>>> + default y
>>>> + select COMMON_CLK_MESON_REGMAP
>>>> + select COMMON_CLK_MESON_DUALDIV
>>>> + select COMMON_CLK_MESON_VID_PLL_DIV
>>>> + help
>>>> + Support for the Peripherals clock controller on Amlogic S805X2 and S905Y4
>>>> + devices, aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
>>>> + Say Y if you want peripherals to work.
>>>> endmenu
>>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>>> index 376f49cc13f1..c9130afccb48 100644
>>>> --- a/drivers/clk/meson/Makefile
>>>> +++ b/drivers/clk/meson/Makefile
>>>> @@ -20,3 +20,4 @@ obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
>>>> obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
>>>> obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
>>>> obj-$(CONFIG_COMMON_CLK_S4_PLL) += s4-pll.o
>>>> +obj-$(CONFIG_COMMON_CLK_S4) += s4-peripherals.o
>>> [...]
>>
>> ?
>>
>>>
>>>> +static struct clk_regmap s4_ceca_32k_clkin = {
>>>> + .data = &(struct clk_regmap_gate_data){
>>>> + .offset = CLKCTRL_CECA_CTRL0,
>>>> + .bit_idx = 31,
>>>> + },
>>>> + .hw.init = &(struct clk_init_data) {
>>>> + .name = "ceca_32k_clkin",
>>>> + .ops = &clk_regmap_gate_ops,
>>>> + .parent_data = (const struct clk_parent_data []) {
>>>> + { .fw_name = "xtal", }
>>>> + },
>>>> + .num_parents = 1,
>>>> + },
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_ceca_32k_div = {
>>>> + .data = &(struct meson_clk_dualdiv_data){
>>>> + .n1 = {
>>>> + .reg_off = CLKCTRL_CECA_CTRL0,
>>>> + .shift = 0,
>>>> + .width = 12,
>>>> + },
>>>> + .n2 = {
>>>> + .reg_off = CLKCTRL_CECA_CTRL0,
>>>> + .shift = 12,
>>>> + .width = 12,
>>>> + },
>>>> + .m1 = {
>>>> + .reg_off = CLKCTRL_CECA_CTRL1,
>>>> + .shift = 0,
>>>> + .width = 12,
>>>> + },
>>>> + .m2 = {
>>>> + .reg_off = CLKCTRL_CECA_CTRL1,
>>>> + .shift = 12,
>>>> + .width = 12,
>>>> + },
>>>> + .dual = {
>>>> + .reg_off = CLKCTRL_CECA_CTRL0,
>>>> + .shift = 28,
>>>> + .width = 1,
>>>> + },
>>>> + .table = s4_32k_div_table,
>>>> + },
>>>> + .hw.init = &(struct clk_init_data){
>>>> + .name = "ceca_32k_div",
>>>> + .ops = &meson_clk_dualdiv_ops,
>>>> + .parent_hws = (const struct clk_hw *[]) {
>>>> + &s4_ceca_32k_clkin.hw
>>>> + },
>>>> + .num_parents = 1,
>>>> + },
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_ceca_32k_sel_pre = {
>>>> + .data = &(struct clk_regmap_mux_data) {
>>>> + .offset = CLKCTRL_CECA_CTRL1,
>>>> + .mask = 0x1,
>>>> + .shift = 24,
>>>> + .flags = CLK_MUX_ROUND_CLOSEST,
>>>> + },
>>>> + .hw.init = &(struct clk_init_data){
>>>> + .name = "ceca_32k_sel_pre",
>>>> + .ops = &clk_regmap_mux_ops,
>>>> + .parent_hws = (const struct clk_hw *[]) {
>>>> + &s4_ceca_32k_div.hw,
>>>> + &s4_ceca_32k_clkin.hw
>>>> + },
>>>> + .num_parents = 2,
>>>> + .flags = CLK_SET_RATE_PARENT,
>>>> + },
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_ceca_32k_sel = {
>>>> + .data = &(struct clk_regmap_mux_data) {
>>>> + .offset = CLKCTRL_CECA_CTRL1,
>>>> + .mask = 0x1,
>>>> + .shift = 31,
>>>> + .flags = CLK_MUX_ROUND_CLOSEST,
>>>> + },
>>>> + .hw.init = &(struct clk_init_data){
>>>> + .name = "ceca_32k_sel",
>>>> + .ops = &clk_regmap_mux_ops,
>>>> + .parent_hws = (const struct clk_hw *[]) {
>>>> + &s4_ceca_32k_sel_pre.hw,
>>>> + &s4_rtc_clk.hw
>>>> + },
>>>> + .num_parents = 2,
>>>> + .flags = CLK_SET_RATE_PARENT,
>>> In my opinion, all clocks that can inherit from a more accurate RTC clock
>>> should be marked with the CLK_SET_RATE_NO_REPARENT flag.
>>> This is necessary because in certain situations, it may be required to
>>> freeze their parent. The setup of these clocks' parent should be located
>>> on the device tree's side.
>>
>> We don't need to freeze parent,in a real project.
>
> "a real project" to whom ?
Yes. We're using native code. Not upstream code. It's been a few years.
We did it upstream after we were stable on the actual project.
>
> Dmitry remark makes sense to me.
I will add the flag as you and Dmitry suggested.
>
>>
>>> [...]
>>>
>>>> +
>>>> +/*
>>>> + * gen clk is designed for debug/monitor some internal clock quality. Some of the
>>>> + * corresponding clock sources are not described in the clock tree and internal clock
>>>> + * for debug, so they are skipped.
>>>> + */
>>>> +static u32 s4_gen_clk_mux_table[] = { 0, 4, 5, 7, 19, 21, 22,
>>>> + 23, 24, 25, 26, 27, 28 };
>>>> +static const struct clk_parent_data s4_gen_clk_parent_data[] = {
>>>> + { .fw_name = "xtal", },
>>>> + { .hw = &s4_vid_pll.hw },
>>>> + { .fw_name = "gp0_pll", },
>>>> + { .fw_name = "hifi_pll", },
>>>> + { .fw_name = "fclk_div2", },
>>>> + { .fw_name = "fclk_div3", },
>>>> + { .fw_name = "fclk_div4", },
>>>> + { .fw_name = "fclk_div5", },
>>>> + { .fw_name = "fclk_div7", },
>>>> + { .fw_name = "mpll0", },
>>>> + { .fw_name = "mpll1", },
>>>> + { .fw_name = "mpll2", },
>>>> + { .fw_name = "mpll3", },
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_gen_clk_sel = {
>>>> + .data = &(struct clk_regmap_mux_data){
>>>> + .offset = CLKCTRL_GEN_CLK_CTRL,
>>>> + .mask = 0x1f,
>>>> + .shift = 12,
>>>> + .table = s4_gen_clk_mux_table,
>>>> + },
>>>> + .hw.init = &(struct clk_init_data){
>>>> + .name = "gen_clk_sel",
>>>> + .ops = &clk_regmap_mux_ops,
>>>> + .parent_data = s4_gen_clk_parent_data,
>>>> + .num_parents = ARRAY_SIZE(s4_gen_clk_parent_data),
>>> I think, the gen_clk selector should be marked with the
>>> CLK_SET_RATE_NO_REPARENT flag. This is because the GEN clock can be
>>> connected to an external pad and may be set up directly from the
>>> device tree.
>>
>> This is used by the debug table clock and is not connected externally.
>>
>
> Again, Dmitry remark is very interresting.
> This debug clock is typacally one you don't really want to automatically reparent
>
I will add the flag as you and Dmitry suggested.
>>>
>>>> + },
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_gen_clk_div = {
>>>> + .data = &(struct clk_regmap_div_data){
>>>> + .offset = CLKCTRL_GEN_CLK_CTRL,
>>>> + .shift = 0,
>>>> + .width = 11,
>>>> + },
>>>> + .hw.init = &(struct clk_init_data){
>>>> + .name = "gen_clk_div",
>>>> + .ops = &clk_regmap_divider_ops,
>>>> + .parent_hws = (const struct clk_hw *[]) {
>>>> + &s4_gen_clk_sel.hw
>>>> + },
>>>> + .num_parents = 1,
>>>> + .flags = CLK_SET_RATE_PARENT,
>>>> + },
>>>> +};
>>>> +
>>>> +static struct clk_regmap s4_gen_clk = {
>>>> + .data = &(struct clk_regmap_gate_data){
>>>> + .offset = CLKCTRL_GEN_CLK_CTRL,
>>>> + .bit_idx = 11,
>>>> + },
>>>> + .hw.init = &(struct clk_init_data) {
>>>> + .name = "gen_clk",
>>>> + .ops = &clk_regmap_gate_ops,
>>>> + .parent_hws = (const struct clk_hw *[]) {
>>>> + &s4_gen_clk_div.hw
>>>> + },
>>>> + .num_parents = 1,
>>>> + .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,
>>>> + },
>>>> +};
>>>> +
>>> [...]
>>> --
>>> Thank you,
>>> Dmitry
>
Powered by blists - more mailing lists