| 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: <d23885a5-6d42-443a-bf19-eb6747e8ec47@gmail.com>
Date: Mon, 15 Sep 2025 11:59:47 +0300
From: Ivaylo Ivanov <ivo.ivanov.ivanov1@...il.com>
To: Peng Fan <peng.fan@....nxp.com>
Cc: Krzysztof Kozlowski <krzk@...nel.org>,
Sylwester Nawrocki <s.nawrocki@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>, Alim Akhtar <alim.akhtar@...sung.com>,
Michael Turquette <mturquette@...libre.com>, Stephen Boyd
<sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-samsung-soc@...r.kernel.org,
devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 5/5] clk: samsung: introduce exynos8890 clock driver
On 9/15/25 10:49, Peng Fan wrote:
> On Sun, Sep 14, 2025 at 03:21:16PM +0300, Ivaylo Ivanov wrote:
>> Introduce a clocks management driver for exynos8890, providing clocks
>> for the peripherals of that SoC.
>>
>> As exynos8890 is the first SoC to have HWACG, it differs a bit from the
> Hardware Auto Clock Gating(HWACG), if I understand correctly.
>
>> newer SoCs. Q-channel and Q-state bits are separate registers, unlike
>> the CLK_CON_GAT_* ones that feature HWACG bits in the same register
>> that controls manual gating. Hence, don't use the clk-exynos-arm64
>> helper, but implement logic that enforces manual gating according to
>> how HWACG is implemented here.
>>
>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@...il.com>
>> ---
>> drivers/clk/samsung/Makefile | 1 +
>> drivers/clk/samsung/clk-exynos8890.c | 8695 ++++++++++++++++++++++++++
>> 2 files changed, 8696 insertions(+)
>> create mode 100644 drivers/clk/samsung/clk-exynos8890.c
>>
>> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
>> index b77fe288e..982dc7c64 100644
>> --- a/drivers/clk/samsung/Makefile
>> +++ b/drivers/clk/samsung/Makefile
>> @@ -22,6 +22,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos7.o
>> obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos7870.o
>> obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos7885.o
>> obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos850.o
>> +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos8890.o
>> obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos8895.o
>> obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos990.o
>> obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynosautov9.o
>> diff --git a/drivers/clk/samsung/clk-exynos8890.c b/drivers/clk/samsung/clk-exynos8890.c
>> new file mode 100644
>> index 000000000..670587bae
>> --- /dev/null
>> +++ b/drivers/clk/samsung/clk-exynos8890.c
>> @@ -0,0 +1,8695 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2025 Ivaylo Ivanov <ivo.ivanov.ivanov1@...il.com>
>> + * Author: Ivaylo Ivanov <ivo.ivanov.ivanov1@...il.com>
>> + *
>> + * Common Clock Framework support for Exynos8890 SoC.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include <dt-bindings/clock/samsung,exynos8890-cmu.h>
>> +
>> +#include "clk.h"
>> +
>> +/* NOTE: Must be equal to the last clock ID increased by one */
>> +#define TOP_NR_CLK (CLK_GOUT_TOP_SCLK_PROMISE_DISP + 1)
>> +#define PERIS_NR_CLK (CLK_GOUT_PERIS_SCLK_PROMISE_PERIS + 1)
>> +#define APOLLO_NR_CLK (CLK_GOUT_APOLLO_SCLK_PROMISE_APOLLO + 1)
>> +#define AUD_NR_CLK (CLK_GOUT_AUD_SCLK_I2S_BCLK + 1)
>> +#define BUS0_NR_CLK (CLK_GOUT_BUS0_ACLK_TREX_P_BUS0 + 1)
>> +#define BUS1_NR_CLK (CLK_GOUT_BUS1_ACLK_TREX_P_BUS1 + 1)
>> +#define CCORE_NR_CLK (CLK_GOUT_CCORE_SCLK_PROMISE + 1)
>> +#define DISP0_NR_CLK (CLK_GOUT_DISP0_OSCCLK_DP_I_CLK_24M + 1)
>> +#define DISP1_NR_CLK (CLK_GOUT_DISP1_SCLK_PROMISE_DISP1 + 1)
>> +#define FSYS0_NR_CLK (CLK_GOUT_FSYS0_SCLK_USBHOST20_REF_CLK + 1)
>> +#define FSYS1_NR_CLK (CLK_GOUT_FSYS1_SCLK_PROMISE_FSYS1 + 1)
>> +#define G3D_NR_CLK (CLK_GOUT_G3D_SCLK_ASYNCAXI_G3D + 1)
>> +#define MIF0_NR_CLK (CLK_GOUT_MIF0_RCLK_DREX + 1)
>> +#define MIF1_NR_CLK (CLK_GOUT_MIF1_RCLK_DREX + 1)
>> +#define MIF2_NR_CLK (CLK_GOUT_MIF2_RCLK_DREX + 1)
>> +#define MIF3_NR_CLK (CLK_GOUT_MIF3_RCLK_DREX + 1)
>> +#define MNGS_NR_CLK (CLK_GOUT_MNGS_SCLK_PROMISE0_MNGS + 1)
>> +#define PERIC0_NR_CLK (CLK_GOUT_PERIC0_SCLK_PWM + 1)
>> +#define PERIC1_NR_CLK (CLK_GOUT_PERIC1_SCLK_UART5 + 1)
>> +
>> +/*
>> + * As exynos8890 first introduced hwacg, cmu registers are mapped similarly
>> + * to exynos7, with the exception of the new q-state and q-ch registers that
>> + * can set the behavior of automatic gates.
>> + */
>> +
>> +/* decoded magic number from downstream */
>> +#define QCH_EN_MASK BIT(0)
>> +#define QCH_MASK (GENMASK(19, 16) | BIT(12))
>> +#define QCH_DIS (QCH_MASK | FIELD_PREP(QCH_EN_MASK, 0))
> Nit: align code.
Aligned in my editor, patch files offset each line with a single symbol
so formatting gets broken...
>
>> +
>> +/* q-channel registers offsets range */
>> +#define QCH_OFF_START 0x2000
>> +#define QCH_OFF_END 0x23ff
>> +
>> +/* q-state registers offsets range */
>> +#define QSTATE_OFF_START 0x2400
>> +#define QSTATE_OFF_END 0x2fff
> Nit: Align.
What?
>
>> +
>> +/* check if the register offset is a QCH register */
>> +static bool is_qch_reg(unsigned long off)
>> +{
>> + return off >= QCH_OFF_START && off <= QCH_OFF_END;
>> +}
>> +
>> +/* check if the register offset is a QSTATE register */
>> +static bool is_qstate_reg(unsigned long off)
>> +{
>> + return off >= QSTATE_OFF_START && off <= QSTATE_OFF_END;
>> +}
>> +
>> +static void __init exynos8890_init_clocks(struct device_node *np,
>> + const struct samsung_cmu_info *cmu)
>> +{
>> + const unsigned long *reg_offs = cmu->clk_regs;
>> + size_t reg_offs_len = cmu->nr_clk_regs;
>> + void __iomem *reg_base;
>> + size_t i;
>> +
>> + reg_base = of_iomap(np, 0);
>> + if (!reg_base)
>> + panic("%s: failed to map registers\n", __func__);
>> +
>> + for (i = 0; i < reg_offs_len; ++i) {
>> + void __iomem *reg = reg_base + reg_offs[i];
>> + u32 val;
>> +
>> + if (is_qch_reg(reg_offs[i])) {
>> + val = QCH_DIS;
>> + writel(val, reg);
>> + } else if (is_qstate_reg(reg_offs[i])) {
>> + val = 0;
>> + writel(val, reg);
>> + }
> This seems to disable qchannel and set qstate to 0 for disable HWACG.
> If this is true, a comment is preferred.
I believe the "DIS" part is pretty self explanatory, no?
>
>> + }
>> +
>> + iounmap(reg_base);
>> +}
>> +
>> +/* ---- CMU_TOP ------------------------------------------------------------- */
>> +
>> +#define MIF_CLK_CTRL1 0x1084
>> +#define MIF_CLK_CTRL2 0x1088
>> +#define MIF_CLK_CTRL3 0x108C
>> +#define MIF_CLK_CTRL4 0x1090
>> +#define ACD_PSCDC_CTRL_0 0x1094
>> +#define ACD_PSCDC_CTRL_1 0x1098
>> +#define ACD_PSCDC_STAT 0x109C
>> +#define CMU_TOP_SPARE0 0x1100
>> +#define CMU_TOP_SPARE1 0x1104
>> +#define CMU_TOP_SPARE2 0x1108
>> +#define CMU_TOP_SPARE3 0x110C
> Some of the registers not aligned.
How are they not, they're aligned both in editors and the patch. Please elaborate.
>
>> +
> [...]
>> +static void __init exynos8890_cmu_top_init(struct device_node *np)
>> +{
>> + exynos8890_init_clocks(np, &top_cmu_info);
>> + samsung_cmu_register_one(np, &top_cmu_info);
>> +}
>> +
>> +/* Register CMU_TOP early, as it's a dependency for other early domains */
>> +CLK_OF_DECLARE(exynos8890_cmu_top, "samsung,exynos8890-cmu-top",
>> + exynos8890_cmu_top_init);
> Not sure you need to run Android GKI, without module built, this platform
> will not able to support GKI.
>
> It would be better to update to use platform drivers.
Same as what Krzysztof said, design choice accross all samsung clock drivers.
>
>> +
>> +/* ---- CMU_PERIS ---------------------------------------------------------- */
>> +
>> +#define QSTATE_CTRL_TMU 0x2474
>> +#define QSTATE_CTRL_CHIPID 0x2484
>> +#define QSTATE_CTRL_PROMISE_PERIS 0x2488
> Not aligned.
>
>> +
>> +
>> +/* Register CMU_PERIS early, as it's needed for MCT timer */
>> +CLK_OF_DECLARE(exynos8890_cmu_peris, "samsung,exynos8890-cmu-peris",
>> + exynos8890_cmu_peris_init);
> Same as above.
>
>> +
>> +/* ---- CMU_APOLLO --------------------------------------------------------- */
>> +
>> +/* Register Offset definitions for CMU_APOLLO (0x11900000) */
>> +#define APOLLO_PLL_LOCK 0x0000
>> +#define APOLLO_PLL_CON0 0x0100
>> +#define APOLLO_PLL_CON1 0x0104
>> +#define APOLLO_PLL_FREQ_DET 0x010C
> Not align.
Same as the other comments about alignment.
Best regards,
Ivaylo
>
> Regards
> Peng
>
Powered by blists - more mailing lists