* [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 19:56 ` Stephen Boyd
2024-12-03 13:41 ` [RFC PATCH v1 02/14] dt-bindings: clock: thead,th1520: Rename header file Michal Wilczynski
` (12 subsequent siblings)
13 siblings, 1 reply; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
The T-Head TH1520 SoC includes various clocks for different subsystems
like Application Processor (AP) and Video Output (VO) [1]. Currently, the
clock driver only implements AP clocks.
Since the programming interface for these clocks is identical across
subsystems, refactor the code to move common functions into
clk-th1520.c. This prepares the driver to support VO clocks by reducing
code duplication and improving maintainability.
No functional changes are introduced with this refactoring.
Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf [1]
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
MAINTAINERS | 2 +-
drivers/clk/thead/Makefile | 2 +-
drivers/clk/thead/clk-th1520-ap.c | 301 +-----------------------------
drivers/clk/thead/clk-th1520.c | 188 +++++++++++++++++++
drivers/clk/thead/clk-th1520.h | 134 +++++++++++++
5 files changed, 326 insertions(+), 301 deletions(-)
create mode 100644 drivers/clk/thead/clk-th1520.c
create mode 100644 drivers/clk/thead/clk-th1520.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e930c7a58b1..7c85abf1dd1e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20188,7 +20188,7 @@ F: Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
F: Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
F: Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
F: arch/riscv/boot/dts/thead/
-F: drivers/clk/thead/clk-th1520-ap.c
+F: drivers/clk/thead/
F: drivers/mailbox/mailbox-th1520.c
F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
F: drivers/pinctrl/pinctrl-th1520.c
diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
index 7ee0bec1f251..d7cf88390b69 100644
--- a/drivers/clk/thead/Makefile
+++ b/drivers/clk/thead/Makefile
@@ -1,2 +1,2 @@
# SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o
+obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
index 17e32ae08720..a6015805b859 100644
--- a/drivers/clk/thead/clk-th1520-ap.c
+++ b/drivers/clk/thead/clk-th1520-ap.c
@@ -5,297 +5,9 @@
* Authors: Yangtao Li <frank.li@vivo.com>
*/
-#include <dt-bindings/clock/thead,th1520-clk-ap.h>
-#include <linux/bitfield.h>
-#include <linux/clk-provider.h>
-#include <linux/device.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/regmap.h>
-
-#define TH1520_PLL_POSTDIV2 GENMASK(26, 24)
-#define TH1520_PLL_POSTDIV1 GENMASK(22, 20)
-#define TH1520_PLL_FBDIV GENMASK(19, 8)
-#define TH1520_PLL_REFDIV GENMASK(5, 0)
-#define TH1520_PLL_BYPASS BIT(30)
-#define TH1520_PLL_DSMPD BIT(24)
-#define TH1520_PLL_FRAC GENMASK(23, 0)
-#define TH1520_PLL_FRAC_BITS 24
-
-struct ccu_internal {
- u8 shift;
- u8 width;
-};
-
-struct ccu_div_internal {
- u8 shift;
- u8 width;
- u32 flags;
-};
-
-struct ccu_common {
- int clkid;
- struct regmap *map;
- u16 cfg0;
- u16 cfg1;
- struct clk_hw hw;
-};
-
-struct ccu_mux {
- struct ccu_internal mux;
- struct ccu_common common;
-};
-
-struct ccu_gate {
- u32 enable;
- struct ccu_common common;
-};
-
-struct ccu_div {
- u32 enable;
- struct ccu_div_internal div;
- struct ccu_internal mux;
- struct ccu_common common;
-};
-
-struct ccu_pll {
- struct ccu_common common;
-};
-
-#define TH_CCU_ARG(_shift, _width) \
- { \
- .shift = _shift, \
- .width = _width, \
- }
-
-#define TH_CCU_DIV_FLAGS(_shift, _width, _flags) \
- { \
- .shift = _shift, \
- .width = _width, \
- .flags = _flags, \
- }
-
-#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
- struct ccu_gate _struct = { \
- .enable = _gate, \
- .common = { \
- .clkid = _clkid, \
- .cfg0 = _reg, \
- .hw.init = CLK_HW_INIT_PARENTS_DATA( \
- _name, \
- _parent, \
- &clk_gate_ops, \
- _flags), \
- } \
- }
-
-static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
-{
- return container_of(hw, struct ccu_common, hw);
-}
-
-static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
-{
- struct ccu_common *common = hw_to_ccu_common(hw);
-
- return container_of(common, struct ccu_mux, common);
-}
-
-static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
-{
- struct ccu_common *common = hw_to_ccu_common(hw);
+#include "clk-th1520.h"
- return container_of(common, struct ccu_pll, common);
-}
-
-static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
-{
- struct ccu_common *common = hw_to_ccu_common(hw);
-
- return container_of(common, struct ccu_div, common);
-}
-
-static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
-{
- struct ccu_common *common = hw_to_ccu_common(hw);
-
- return container_of(common, struct ccu_gate, common);
-}
-
-static u8 ccu_get_parent_helper(struct ccu_common *common,
- struct ccu_internal *mux)
-{
- unsigned int val;
- u8 parent;
-
- regmap_read(common->map, common->cfg0, &val);
- parent = val >> mux->shift;
- parent &= GENMASK(mux->width - 1, 0);
-
- return parent;
-}
-
-static int ccu_set_parent_helper(struct ccu_common *common,
- struct ccu_internal *mux,
- u8 index)
-{
- return regmap_update_bits(common->map, common->cfg0,
- GENMASK(mux->width - 1, 0) << mux->shift,
- index << mux->shift);
-}
-
-static void ccu_disable_helper(struct ccu_common *common, u32 gate)
-{
- if (!gate)
- return;
- regmap_update_bits(common->map, common->cfg0,
- gate, ~gate);
-}
-
-static int ccu_enable_helper(struct ccu_common *common, u32 gate)
-{
- unsigned int val;
- int ret;
-
- if (!gate)
- return 0;
-
- ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
- regmap_read(common->map, common->cfg0, &val);
- return ret;
-}
-
-static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
-{
- unsigned int val;
-
- if (!gate)
- return true;
-
- regmap_read(common->map, common->cfg0, &val);
- return val & gate;
-}
-
-static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
- unsigned long parent_rate)
-{
- struct ccu_div *cd = hw_to_ccu_div(hw);
- unsigned long rate;
- unsigned int val;
-
- regmap_read(cd->common.map, cd->common.cfg0, &val);
- val = val >> cd->div.shift;
- val &= GENMASK(cd->div.width - 1, 0);
- rate = divider_recalc_rate(hw, parent_rate, val, NULL,
- cd->div.flags, cd->div.width);
-
- return rate;
-}
-
-static u8 ccu_div_get_parent(struct clk_hw *hw)
-{
- struct ccu_div *cd = hw_to_ccu_div(hw);
-
- return ccu_get_parent_helper(&cd->common, &cd->mux);
-}
-
-static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
-{
- struct ccu_div *cd = hw_to_ccu_div(hw);
-
- return ccu_set_parent_helper(&cd->common, &cd->mux, index);
-}
-
-static void ccu_div_disable(struct clk_hw *hw)
-{
- struct ccu_div *cd = hw_to_ccu_div(hw);
-
- ccu_disable_helper(&cd->common, cd->enable);
-}
-
-static int ccu_div_enable(struct clk_hw *hw)
-{
- struct ccu_div *cd = hw_to_ccu_div(hw);
-
- return ccu_enable_helper(&cd->common, cd->enable);
-}
-
-static int ccu_div_is_enabled(struct clk_hw *hw)
-{
- struct ccu_div *cd = hw_to_ccu_div(hw);
-
- return ccu_is_enabled_helper(&cd->common, cd->enable);
-}
-
-static const struct clk_ops ccu_div_ops = {
- .disable = ccu_div_disable,
- .enable = ccu_div_enable,
- .is_enabled = ccu_div_is_enabled,
- .get_parent = ccu_div_get_parent,
- .set_parent = ccu_div_set_parent,
- .recalc_rate = ccu_div_recalc_rate,
- .determine_rate = clk_hw_determine_rate_no_reparent,
-};
-
-static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
- unsigned long parent_rate)
-{
- struct ccu_pll *pll = hw_to_ccu_pll(hw);
- unsigned long div, mul, frac;
- unsigned int cfg0, cfg1;
- u64 rate = parent_rate;
-
- regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
- regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
-
- mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
- div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
- if (!(cfg1 & TH1520_PLL_DSMPD)) {
- mul <<= TH1520_PLL_FRAC_BITS;
- frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
- mul += frac;
- div <<= TH1520_PLL_FRAC_BITS;
- }
- rate = parent_rate * mul;
- rate = rate / div;
- return rate;
-}
-
-static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
- unsigned long parent_rate)
-{
- struct ccu_pll *pll = hw_to_ccu_pll(hw);
- unsigned long div, rate = parent_rate;
- unsigned int cfg0, cfg1;
-
- regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
- regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
-
- if (cfg1 & TH1520_PLL_BYPASS)
- return rate;
-
- div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
- FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
-
- rate = rate / div;
-
- return rate;
-}
-
-static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
- unsigned long parent_rate)
-{
- unsigned long rate = parent_rate;
-
- rate = th1520_pll_vco_recalc_rate(hw, rate);
- rate = th1520_pll_postdiv_recalc_rate(hw, rate);
-
- return rate;
-}
-
-static const struct clk_ops clk_pll_ops = {
- .recalc_rate = ccu_pll_recalc_rate,
-};
+#define NR_CLKS (CLK_UART_SCLK + 1)
static const struct clk_parent_data osc_24m_clk[] = {
{ .index = 0 }
@@ -956,15 +668,6 @@ static struct ccu_common *th1520_gate_clks[] = {
&sram3_clk.common,
};
-#define NR_CLKS (CLK_UART_SCLK + 1)
-
-static const struct regmap_config th1520_clk_regmap_config = {
- .reg_bits = 32,
- .val_bits = 32,
- .reg_stride = 4,
- .fast_io = true,
-};
-
static int th1520_clk_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
diff --git a/drivers/clk/thead/clk-th1520.c b/drivers/clk/thead/clk-th1520.c
new file mode 100644
index 000000000000..e2bfe56de9af
--- /dev/null
+++ b/drivers/clk/thead/clk-th1520.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
+ * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
+ * Authors: Yangtao Li <frank.li@vivo.com>
+ */
+
+#include "clk-th1520.h"
+
+static u8 ccu_get_parent_helper(struct ccu_common *common,
+ struct ccu_internal *mux)
+{
+ unsigned int val;
+ u8 parent;
+
+ regmap_read(common->map, common->cfg0, &val);
+ parent = val >> mux->shift;
+ parent &= GENMASK(mux->width - 1, 0);
+
+ return parent;
+}
+
+static int ccu_set_parent_helper(struct ccu_common *common,
+ struct ccu_internal *mux, u8 index)
+{
+ return regmap_update_bits(common->map, common->cfg0,
+ GENMASK(mux->width - 1, 0) << mux->shift,
+ index << mux->shift);
+}
+
+static void ccu_disable_helper(struct ccu_common *common, u32 gate)
+{
+ if (!gate)
+ return;
+ regmap_update_bits(common->map, common->cfg0, gate, ~gate);
+}
+
+static int ccu_enable_helper(struct ccu_common *common, u32 gate)
+{
+ unsigned int val;
+ int ret;
+
+ if (!gate)
+ return 0;
+
+ ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
+ regmap_read(common->map, common->cfg0, &val);
+ return ret;
+}
+
+static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
+{
+ unsigned int val;
+
+ if (!gate)
+ return true;
+
+ regmap_read(common->map, common->cfg0, &val);
+ return val & gate;
+}
+
+static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct ccu_div *cd = hw_to_ccu_div(hw);
+ unsigned long rate;
+ unsigned int val;
+
+ regmap_read(cd->common.map, cd->common.cfg0, &val);
+ val = val >> cd->div.shift;
+ val &= GENMASK(cd->div.width - 1, 0);
+ rate = divider_recalc_rate(hw, parent_rate, val, NULL, cd->div.flags,
+ cd->div.width);
+
+ return rate;
+}
+
+static u8 ccu_div_get_parent(struct clk_hw *hw)
+{
+ struct ccu_div *cd = hw_to_ccu_div(hw);
+
+ return ccu_get_parent_helper(&cd->common, &cd->mux);
+}
+
+static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
+{
+ struct ccu_div *cd = hw_to_ccu_div(hw);
+
+ return ccu_set_parent_helper(&cd->common, &cd->mux, index);
+}
+
+static void ccu_div_disable(struct clk_hw *hw)
+{
+ struct ccu_div *cd = hw_to_ccu_div(hw);
+
+ ccu_disable_helper(&cd->common, cd->enable);
+}
+
+static int ccu_div_enable(struct clk_hw *hw)
+{
+ struct ccu_div *cd = hw_to_ccu_div(hw);
+
+ return ccu_enable_helper(&cd->common, cd->enable);
+}
+
+static int ccu_div_is_enabled(struct clk_hw *hw)
+{
+ struct ccu_div *cd = hw_to_ccu_div(hw);
+
+ return ccu_is_enabled_helper(&cd->common, cd->enable);
+}
+
+const struct clk_ops ccu_div_ops = {
+ .disable = ccu_div_disable,
+ .enable = ccu_div_enable,
+ .is_enabled = ccu_div_is_enabled,
+ .get_parent = ccu_div_get_parent,
+ .set_parent = ccu_div_set_parent,
+ .recalc_rate = ccu_div_recalc_rate,
+ .determine_rate = clk_hw_determine_rate_no_reparent,
+};
+
+static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct ccu_pll *pll = hw_to_ccu_pll(hw);
+ unsigned long div, mul, frac;
+ unsigned int cfg0, cfg1;
+ u64 rate = parent_rate;
+
+ regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
+ regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
+
+ mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
+ div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
+ if (!(cfg1 & TH1520_PLL_DSMPD)) {
+ mul <<= TH1520_PLL_FRAC_BITS;
+ frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
+ mul += frac;
+ div <<= TH1520_PLL_FRAC_BITS;
+ }
+ rate = parent_rate * mul;
+ rate = rate / div;
+ return rate;
+}
+
+static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct ccu_pll *pll = hw_to_ccu_pll(hw);
+ unsigned long div, rate = parent_rate;
+ unsigned int cfg0, cfg1;
+
+ regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
+ regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
+
+ if (cfg1 & TH1520_PLL_BYPASS)
+ return rate;
+
+ div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
+ FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
+
+ rate = rate / div;
+
+ return rate;
+}
+
+static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ unsigned long rate = parent_rate;
+
+ rate = th1520_pll_vco_recalc_rate(hw, rate);
+ rate = th1520_pll_postdiv_recalc_rate(hw, rate);
+
+ return rate;
+}
+
+const struct clk_ops clk_pll_ops = {
+ .recalc_rate = ccu_pll_recalc_rate,
+};
+
+const struct regmap_config th1520_clk_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .fast_io = true,
+};
diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
new file mode 100644
index 000000000000..285d41e65008
--- /dev/null
+++ b/drivers/clk/thead/clk-th1520.h
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
+ * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
+ * Authors: Yangtao Li <frank.li@vivo.com>
+ *
+ * clk-th1520.h - Common definitions for T-HEAD TH1520 Clock Drivers
+ */
+
+#ifndef CLK_TH1520_H
+#define CLK_TH1520_H
+
+#include <dt-bindings/clock/thead,th1520-clk-ap.h>
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define TH1520_PLL_POSTDIV2 GENMASK(26, 24)
+#define TH1520_PLL_POSTDIV1 GENMASK(22, 20)
+#define TH1520_PLL_FBDIV GENMASK(19, 8)
+#define TH1520_PLL_REFDIV GENMASK(5, 0)
+#define TH1520_PLL_BYPASS BIT(30)
+#define TH1520_PLL_DSMPD BIT(24)
+#define TH1520_PLL_FRAC GENMASK(23, 0)
+#define TH1520_PLL_FRAC_BITS 24
+
+struct ccu_internal {
+ u8 shift;
+ u8 width;
+};
+
+struct ccu_div_internal {
+ u8 shift;
+ u8 width;
+ u32 flags;
+};
+
+struct ccu_common {
+ int clkid;
+ struct regmap *map;
+ u16 cfg0;
+ u16 cfg1;
+ struct clk_hw hw;
+};
+
+struct ccu_mux {
+ struct ccu_internal mux;
+ struct ccu_common common;
+};
+
+struct ccu_gate {
+ u32 enable;
+ struct ccu_common common;
+};
+
+struct ccu_div {
+ u32 enable;
+ struct ccu_div_internal div;
+ struct ccu_internal mux;
+ struct ccu_common common;
+};
+
+struct ccu_pll {
+ struct ccu_common common;
+};
+
+#define TH_CCU_ARG(_shift, _width) \
+ { \
+ .shift = _shift, \
+ .width = _width, \
+ }
+
+#define TH_CCU_DIV_FLAGS(_shift, _width, _flags) \
+ { \
+ .shift = _shift, \
+ .width = _width, \
+ .flags = _flags, \
+ }
+
+#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
+ struct ccu_gate _struct = { \
+ .enable = _gate, \
+ .common = { \
+ .clkid = _clkid, \
+ .cfg0 = _reg, \
+ .hw.init = CLK_HW_INIT_PARENTS_DATA( \
+ _name, \
+ _parent, \
+ &clk_gate_ops, \
+ _flags), \
+ } \
+ }
+
+static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
+{
+ return container_of(hw, struct ccu_common, hw);
+}
+
+static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
+{
+ struct ccu_common *common = hw_to_ccu_common(hw);
+
+ return container_of(common, struct ccu_mux, common);
+}
+
+static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
+{
+ struct ccu_common *common = hw_to_ccu_common(hw);
+
+ return container_of(common, struct ccu_pll, common);
+}
+
+static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
+{
+ struct ccu_common *common = hw_to_ccu_common(hw);
+
+ return container_of(common, struct ccu_div, common);
+}
+
+static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
+{
+ struct ccu_common *common = hw_to_ccu_common(hw);
+
+ return container_of(common, struct ccu_gate, common);
+}
+
+extern const struct clk_ops ccu_div_ops;
+extern const struct clk_ops clk_pll_ops;
+extern const struct regmap_config th1520_clk_regmap_config;
+
+#endif /* CLK_TH1520_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code
2024-12-03 13:41 ` [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code Michal Wilczynski
@ 2024-12-03 19:56 ` Stephen Boyd
2024-12-04 13:54 ` Michal Wilczynski
0 siblings, 1 reply; 38+ messages in thread
From: Stephen Boyd @ 2024-12-03 19:56 UTC (permalink / raw)
To: Michal Wilczynski, airlied, aou, conor+dt, drew, frank.binns,
guoren, jassisinghbrar, jszhang, krzk+dt, m.szyprowski,
maarten.lankhorst, matt.coster, mripard, mturquette, palmer,
paul.walmsley, robh, simona, tzimmermann, ulf.hansson, wefu
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
Quoting Michal Wilczynski (2024-12-03 05:41:24)
> diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
> index 7ee0bec1f251..d7cf88390b69 100644
> --- a/drivers/clk/thead/Makefile
> +++ b/drivers/clk/thead/Makefile
> @@ -1,2 +1,2 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o
> +obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
Can the -ap driver be extended instead? Or are the clks in a different
IO region?
> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> index 17e32ae08720..a6015805b859 100644
> --- a/drivers/clk/thead/clk-th1520-ap.c
> +++ b/drivers/clk/thead/clk-th1520-ap.c
> @@ -5,297 +5,9 @@
> * Authors: Yangtao Li <frank.li@vivo.com>
> */
>
> -#include <dt-bindings/clock/thead,th1520-clk-ap.h>
Presumably this should stay here.
> -#include <linux/bitfield.h>
> -#include <linux/clk-provider.h>
> -#include <linux/device.h>
> -#include <linux/module.h>
> -#include <linux/platform_device.h>
> -#include <linux/regmap.h>
These should all stay here as well.
> -
> -#define TH1520_PLL_POSTDIV2 GENMASK(26, 24)
> -#define TH1520_PLL_POSTDIV1 GENMASK(22, 20)
> -#define TH1520_PLL_FBDIV GENMASK(19, 8)
> -#define TH1520_PLL_REFDIV GENMASK(5, 0)
> -#define TH1520_PLL_BYPASS BIT(30)
> -#define TH1520_PLL_DSMPD BIT(24)
> -#define TH1520_PLL_FRAC GENMASK(23, 0)
> -#define TH1520_PLL_FRAC_BITS 24
> -
> -struct ccu_internal {
> - u8 shift;
> - u8 width;
> -};
> -
> -struct ccu_div_internal {
> - u8 shift;
> - u8 width;
> - u32 flags;
> -};
> -
> -struct ccu_common {
> - int clkid;
> - struct regmap *map;
> - u16 cfg0;
> - u16 cfg1;
> - struct clk_hw hw;
> -};
> -
> -struct ccu_mux {
> - struct ccu_internal mux;
> - struct ccu_common common;
> -};
> -
> -struct ccu_gate {
> - u32 enable;
> - struct ccu_common common;
> -};
> -
> -struct ccu_div {
> - u32 enable;
> - struct ccu_div_internal div;
> - struct ccu_internal mux;
> - struct ccu_common common;
> -};
> -
> -struct ccu_pll {
> - struct ccu_common common;
> -};
> -
> -#define TH_CCU_ARG(_shift, _width) \
> - { \
> - .shift = _shift, \
> - .width = _width, \
> - }
> -
> -#define TH_CCU_DIV_FLAGS(_shift, _width, _flags) \
> - { \
> - .shift = _shift, \
> - .width = _width, \
> - .flags = _flags, \
> - }
> -
> -#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
> - struct ccu_gate _struct = { \
> - .enable = _gate, \
> - .common = { \
> - .clkid = _clkid, \
> - .cfg0 = _reg, \
> - .hw.init = CLK_HW_INIT_PARENTS_DATA( \
> - _name, \
> - _parent, \
> - &clk_gate_ops, \
> - _flags), \
> - } \
> - }
> -
> -static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
> -{
> - return container_of(hw, struct ccu_common, hw);
> -}
> -
> -static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
> -{
> - struct ccu_common *common = hw_to_ccu_common(hw);
> -
> - return container_of(common, struct ccu_mux, common);
> -}
> -
> -static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
> -{
> - struct ccu_common *common = hw_to_ccu_common(hw);
> +#include "clk-th1520.h"
>
> - return container_of(common, struct ccu_pll, common);
> -}
> -
> -static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
> -{
> - struct ccu_common *common = hw_to_ccu_common(hw);
> -
> - return container_of(common, struct ccu_div, common);
> -}
> -
> -static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
> -{
> - struct ccu_common *common = hw_to_ccu_common(hw);
> -
> - return container_of(common, struct ccu_gate, common);
> -}
> -
> -static u8 ccu_get_parent_helper(struct ccu_common *common,
> - struct ccu_internal *mux)
> -{
> - unsigned int val;
> - u8 parent;
> -
> - regmap_read(common->map, common->cfg0, &val);
> - parent = val >> mux->shift;
> - parent &= GENMASK(mux->width - 1, 0);
> -
> - return parent;
> -}
> -
> -static int ccu_set_parent_helper(struct ccu_common *common,
> - struct ccu_internal *mux,
> - u8 index)
> -{
> - return regmap_update_bits(common->map, common->cfg0,
> - GENMASK(mux->width - 1, 0) << mux->shift,
> - index << mux->shift);
> -}
> -
> -static void ccu_disable_helper(struct ccu_common *common, u32 gate)
> -{
> - if (!gate)
> - return;
> - regmap_update_bits(common->map, common->cfg0,
> - gate, ~gate);
> -}
> -
> -static int ccu_enable_helper(struct ccu_common *common, u32 gate)
> -{
> - unsigned int val;
> - int ret;
> -
> - if (!gate)
> - return 0;
> -
> - ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
> - regmap_read(common->map, common->cfg0, &val);
> - return ret;
> -}
> -
> -static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
> -{
> - unsigned int val;
> -
> - if (!gate)
> - return true;
> -
> - regmap_read(common->map, common->cfg0, &val);
> - return val & gate;
> -}
> -
> -static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
> - unsigned long parent_rate)
> -{
> - struct ccu_div *cd = hw_to_ccu_div(hw);
> - unsigned long rate;
> - unsigned int val;
> -
> - regmap_read(cd->common.map, cd->common.cfg0, &val);
> - val = val >> cd->div.shift;
> - val &= GENMASK(cd->div.width - 1, 0);
> - rate = divider_recalc_rate(hw, parent_rate, val, NULL,
> - cd->div.flags, cd->div.width);
> -
> - return rate;
> -}
> -
> -static u8 ccu_div_get_parent(struct clk_hw *hw)
> -{
> - struct ccu_div *cd = hw_to_ccu_div(hw);
> -
> - return ccu_get_parent_helper(&cd->common, &cd->mux);
> -}
> -
> -static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
> -{
> - struct ccu_div *cd = hw_to_ccu_div(hw);
> -
> - return ccu_set_parent_helper(&cd->common, &cd->mux, index);
> -}
> -
> -static void ccu_div_disable(struct clk_hw *hw)
> -{
> - struct ccu_div *cd = hw_to_ccu_div(hw);
> -
> - ccu_disable_helper(&cd->common, cd->enable);
> -}
> -
> -static int ccu_div_enable(struct clk_hw *hw)
> -{
> - struct ccu_div *cd = hw_to_ccu_div(hw);
> -
> - return ccu_enable_helper(&cd->common, cd->enable);
> -}
> -
> -static int ccu_div_is_enabled(struct clk_hw *hw)
> -{
> - struct ccu_div *cd = hw_to_ccu_div(hw);
> -
> - return ccu_is_enabled_helper(&cd->common, cd->enable);
> -}
> -
> -static const struct clk_ops ccu_div_ops = {
> - .disable = ccu_div_disable,
> - .enable = ccu_div_enable,
> - .is_enabled = ccu_div_is_enabled,
> - .get_parent = ccu_div_get_parent,
> - .set_parent = ccu_div_set_parent,
> - .recalc_rate = ccu_div_recalc_rate,
> - .determine_rate = clk_hw_determine_rate_no_reparent,
> -};
> -
> -static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
> - unsigned long parent_rate)
> -{
> - struct ccu_pll *pll = hw_to_ccu_pll(hw);
> - unsigned long div, mul, frac;
> - unsigned int cfg0, cfg1;
> - u64 rate = parent_rate;
> -
> - regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> - regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> -
> - mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
> - div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
> - if (!(cfg1 & TH1520_PLL_DSMPD)) {
> - mul <<= TH1520_PLL_FRAC_BITS;
> - frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
> - mul += frac;
> - div <<= TH1520_PLL_FRAC_BITS;
> - }
> - rate = parent_rate * mul;
> - rate = rate / div;
> - return rate;
> -}
> -
> -static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
> - unsigned long parent_rate)
> -{
> - struct ccu_pll *pll = hw_to_ccu_pll(hw);
> - unsigned long div, rate = parent_rate;
> - unsigned int cfg0, cfg1;
> -
> - regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> - regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> -
> - if (cfg1 & TH1520_PLL_BYPASS)
> - return rate;
> -
> - div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
> - FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
> -
> - rate = rate / div;
> -
> - return rate;
> -}
> -
> -static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
> - unsigned long parent_rate)
> -{
> - unsigned long rate = parent_rate;
> -
> - rate = th1520_pll_vco_recalc_rate(hw, rate);
> - rate = th1520_pll_postdiv_recalc_rate(hw, rate);
> -
> - return rate;
> -}
> -
> -static const struct clk_ops clk_pll_ops = {
> - .recalc_rate = ccu_pll_recalc_rate,
> -};
> +#define NR_CLKS (CLK_UART_SCLK + 1)
>
> static const struct clk_parent_data osc_24m_clk[] = {
> { .index = 0 }
> @@ -956,15 +668,6 @@ static struct ccu_common *th1520_gate_clks[] = {
> &sram3_clk.common,
> };
>
> -#define NR_CLKS (CLK_UART_SCLK + 1)
Why did this move?
> -
> -static const struct regmap_config th1520_clk_regmap_config = {
> - .reg_bits = 32,
> - .val_bits = 32,
> - .reg_stride = 4,
> - .fast_io = true,
> -};
> -
> static int th1520_clk_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> diff --git a/drivers/clk/thead/clk-th1520.c b/drivers/clk/thead/clk-th1520.c
> new file mode 100644
> index 000000000000..e2bfe56de9af
> --- /dev/null
> +++ b/drivers/clk/thead/clk-th1520.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
> + * Authors: Yangtao Li <frank.li@vivo.com>
> + */
> +
> +#include "clk-th1520.h"
Explicitly include linux headers here, don't just put them all into a
header file. It helps us easily see what C files are using different
parts of the kernel infrastructure.
> +
> +static u8 ccu_get_parent_helper(struct ccu_common *common,
> + struct ccu_internal *mux)
> +{
> + unsigned int val;
> + u8 parent;
> +
> + regmap_read(common->map, common->cfg0, &val);
> + parent = val >> mux->shift;
> + parent &= GENMASK(mux->width - 1, 0);
> +
> + return parent;
> +}
> +
> +static int ccu_set_parent_helper(struct ccu_common *common,
> + struct ccu_internal *mux, u8 index)
> +{
> + return regmap_update_bits(common->map, common->cfg0,
> + GENMASK(mux->width - 1, 0) << mux->shift,
> + index << mux->shift);
> +}
> +
> +static void ccu_disable_helper(struct ccu_common *common, u32 gate)
> +{
> + if (!gate)
> + return;
> + regmap_update_bits(common->map, common->cfg0, gate, ~gate);
> +}
> +
> +static int ccu_enable_helper(struct ccu_common *common, u32 gate)
> +{
> + unsigned int val;
> + int ret;
> +
> + if (!gate)
> + return 0;
> +
> + ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
> + regmap_read(common->map, common->cfg0, &val);
> + return ret;
> +}
> +
> +static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
> +{
> + unsigned int val;
> +
> + if (!gate)
> + return true;
> +
> + regmap_read(common->map, common->cfg0, &val);
> + return val & gate;
> +}
> +
> +static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct ccu_div *cd = hw_to_ccu_div(hw);
> + unsigned long rate;
> + unsigned int val;
> +
> + regmap_read(cd->common.map, cd->common.cfg0, &val);
> + val = val >> cd->div.shift;
> + val &= GENMASK(cd->div.width - 1, 0);
> + rate = divider_recalc_rate(hw, parent_rate, val, NULL, cd->div.flags,
> + cd->div.width);
> +
> + return rate;
> +}
> +
> +static u8 ccu_div_get_parent(struct clk_hw *hw)
> +{
> + struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> + return ccu_get_parent_helper(&cd->common, &cd->mux);
> +}
> +
> +static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
> +{
> + struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> + return ccu_set_parent_helper(&cd->common, &cd->mux, index);
> +}
> +
> +static void ccu_div_disable(struct clk_hw *hw)
> +{
> + struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> + ccu_disable_helper(&cd->common, cd->enable);
> +}
> +
> +static int ccu_div_enable(struct clk_hw *hw)
> +{
> + struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> + return ccu_enable_helper(&cd->common, cd->enable);
> +}
> +
> +static int ccu_div_is_enabled(struct clk_hw *hw)
> +{
> + struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> + return ccu_is_enabled_helper(&cd->common, cd->enable);
> +}
> +
> +const struct clk_ops ccu_div_ops = {
> + .disable = ccu_div_disable,
> + .enable = ccu_div_enable,
> + .is_enabled = ccu_div_is_enabled,
> + .get_parent = ccu_div_get_parent,
> + .set_parent = ccu_div_set_parent,
> + .recalc_rate = ccu_div_recalc_rate,
> + .determine_rate = clk_hw_determine_rate_no_reparent,
> +};
> +
> +static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct ccu_pll *pll = hw_to_ccu_pll(hw);
> + unsigned long div, mul, frac;
> + unsigned int cfg0, cfg1;
> + u64 rate = parent_rate;
> +
> + regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> + regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> +
> + mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
> + div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
> + if (!(cfg1 & TH1520_PLL_DSMPD)) {
> + mul <<= TH1520_PLL_FRAC_BITS;
> + frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
> + mul += frac;
> + div <<= TH1520_PLL_FRAC_BITS;
> + }
> + rate = parent_rate * mul;
> + rate = rate / div;
> + return rate;
> +}
> +
> +static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct ccu_pll *pll = hw_to_ccu_pll(hw);
> + unsigned long div, rate = parent_rate;
> + unsigned int cfg0, cfg1;
> +
> + regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> + regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> +
> + if (cfg1 & TH1520_PLL_BYPASS)
> + return rate;
> +
> + div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
> + FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
> +
> + rate = rate / div;
> +
> + return rate;
> +}
> +
> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + unsigned long rate = parent_rate;
> +
> + rate = th1520_pll_vco_recalc_rate(hw, rate);
> + rate = th1520_pll_postdiv_recalc_rate(hw, rate);
> +
> + return rate;
> +}
> +
> +const struct clk_ops clk_pll_ops = {
> + .recalc_rate = ccu_pll_recalc_rate,
> +};
> +
> +const struct regmap_config th1520_clk_regmap_config = {
I don't get why this is moved.
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .fast_io = true,
> +};
> diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
> new file mode 100644
> index 000000000000..285d41e65008
> --- /dev/null
> +++ b/drivers/clk/thead/clk-th1520.h
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
> + * Authors: Yangtao Li <frank.li@vivo.com>
> + *
> + * clk-th1520.h - Common definitions for T-HEAD TH1520 Clock Drivers
> + */
> +
> +#ifndef CLK_TH1520_H
> +#define CLK_TH1520_H
> +
> +#include <dt-bindings/clock/thead,th1520-clk-ap.h>
dt-bindings comes after linux includes, but this shouldn't be here
anyway.
> +#include <linux/bitfield.h>
> +#include <linux/clk-provider.h>
Seems we have to have this one for clk_hw.
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
I don't see these includes used here, so remove them.
> +#include <linux/regmap.h>
Forward declare regmap and drop the include
struct regmap;
> +
> +#define TH1520_PLL_POSTDIV2 GENMASK(26, 24)
> +#define TH1520_PLL_POSTDIV1 GENMASK(22, 20)
> +#define TH1520_PLL_FBDIV GENMASK(19, 8)
> +#define TH1520_PLL_REFDIV GENMASK(5, 0)
> +#define TH1520_PLL_BYPASS BIT(30)
> +#define TH1520_PLL_DSMPD BIT(24)
> +#define TH1520_PLL_FRAC GENMASK(23, 0)
> +#define TH1520_PLL_FRAC_BITS 24
Are these going to be used in multiple drivers?
> +
> +struct ccu_internal {
> + u8 shift;
> + u8 width;
> +};
> +
> +struct ccu_div_internal {
> + u8 shift;
> + u8 width;
> + u32 flags;
> +};
> +
> +struct ccu_common {
> + int clkid;
> + struct regmap *map;
> + u16 cfg0;
> + u16 cfg1;
> + struct clk_hw hw;
> +};
> +
> +struct ccu_mux {
> + struct ccu_internal mux;
> + struct ccu_common common;
> +};
> +
> +struct ccu_gate {
> + u32 enable;
> + struct ccu_common common;
> +};
> +
> +struct ccu_div {
> + u32 enable;
> + struct ccu_div_internal div;
> + struct ccu_internal mux;
> + struct ccu_common common;
> +};
> +
> +struct ccu_pll {
> + struct ccu_common common;
> +};
> +
> +#define TH_CCU_ARG(_shift, _width) \
> + { \
> + .shift = _shift, \
> + .width = _width, \
> + }
> +
> +#define TH_CCU_DIV_FLAGS(_shift, _width, _flags) \
> + { \
> + .shift = _shift, \
> + .width = _width, \
> + .flags = _flags, \
> + }
> +
> +#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
> + struct ccu_gate _struct = { \
> + .enable = _gate, \
> + .common = { \
> + .clkid = _clkid, \
> + .cfg0 = _reg, \
> + .hw.init = CLK_HW_INIT_PARENTS_DATA( \
> + _name, \
> + _parent, \
> + &clk_gate_ops, \
> + _flags), \
> + } \
> + }
> +
> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
> +{
> + return container_of(hw, struct ccu_common, hw);
> +}
> +
> +static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
> +{
> + struct ccu_common *common = hw_to_ccu_common(hw);
> +
> + return container_of(common, struct ccu_mux, common);
> +}
> +
> +static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
> +{
> + struct ccu_common *common = hw_to_ccu_common(hw);
> +
> + return container_of(common, struct ccu_pll, common);
> +}
> +
> +static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
> +{
> + struct ccu_common *common = hw_to_ccu_common(hw);
> +
> + return container_of(common, struct ccu_div, common);
> +}
> +
> +static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
> +{
> + struct ccu_common *common = hw_to_ccu_common(hw);
> +
> + return container_of(common, struct ccu_gate, common);
> +}
> +
> +extern const struct clk_ops ccu_div_ops;
> +extern const struct clk_ops clk_pll_ops;
> +extern const struct regmap_config th1520_clk_regmap_config;
Why is the regmap config exported?
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code
2024-12-03 19:56 ` Stephen Boyd
@ 2024-12-04 13:54 ` Michal Wilczynski
2024-12-05 7:31 ` Krzysztof Kozlowski
0 siblings, 1 reply; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-04 13:54 UTC (permalink / raw)
To: Stephen Boyd, airlied, aou, conor+dt, drew, frank.binns, guoren,
jassisinghbrar, jszhang, krzk+dt, m.szyprowski, maarten.lankhorst,
matt.coster, mripard, mturquette, palmer, paul.walmsley, robh,
simona, tzimmermann, ulf.hansson, wefu
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 12/3/24 20:56, Stephen Boyd wrote:
> Quoting Michal Wilczynski (2024-12-03 05:41:24)
>> diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
>> index 7ee0bec1f251..d7cf88390b69 100644
>> --- a/drivers/clk/thead/Makefile
>> +++ b/drivers/clk/thead/Makefile
>> @@ -1,2 +1,2 @@
>> # SPDX-License-Identifier: GPL-2.0
>> -obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o
>> +obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
>
> Can the -ap driver be extended instead? Or are the clks in a different
> IO region?
The Video Output (VO) clocks reside in a different address space as
defined in the T-HEAD manual 4.4.1 [1]. Therefore, creating a separate
driver made sense to maintain clarity and adhere to the existing
convention of having one driver per subsystem, similar to the
AP-specific driver.
[1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
>
>> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
>> index 17e32ae08720..a6015805b859 100644
>> --- a/drivers/clk/thead/clk-th1520-ap.c
>> +++ b/drivers/clk/thead/clk-th1520-ap.c
>> @@ -5,297 +5,9 @@
>> * Authors: Yangtao Li <frank.li@vivo.com>
>> */
>>
>> -#include <dt-bindings/clock/thead,th1520-clk-ap.h>
>
> Presumably this should stay here.
>
>> -#include <linux/bitfield.h>
>> -#include <linux/clk-provider.h>
>> -#include <linux/device.h>
>> -#include <linux/module.h>
>> -#include <linux/platform_device.h>
>> -#include <linux/regmap.h>
>
> These should all stay here as well.
>
>> -
>> -#define TH1520_PLL_POSTDIV2 GENMASK(26, 24)
>> -#define TH1520_PLL_POSTDIV1 GENMASK(22, 20)
>> -#define TH1520_PLL_FBDIV GENMASK(19, 8)
>> -#define TH1520_PLL_REFDIV GENMASK(5, 0)
>> -#define TH1520_PLL_BYPASS BIT(30)
>> -#define TH1520_PLL_DSMPD BIT(24)
>> -#define TH1520_PLL_FRAC GENMASK(23, 0)
>> -#define TH1520_PLL_FRAC_BITS 24
>> -
>> -struct ccu_internal {
>> - u8 shift;
>> - u8 width;
>> -};
>> -
>> -struct ccu_div_internal {
>> - u8 shift;
>> - u8 width;
>> - u32 flags;
>> -};
>> -
>> -struct ccu_common {
>> - int clkid;
>> - struct regmap *map;
>> - u16 cfg0;
>> - u16 cfg1;
>> - struct clk_hw hw;
>> -};
>> -
>> -struct ccu_mux {
>> - struct ccu_internal mux;
>> - struct ccu_common common;
>> -};
>> -
>> -struct ccu_gate {
>> - u32 enable;
>> - struct ccu_common common;
>> -};
>> -
>> -struct ccu_div {
>> - u32 enable;
>> - struct ccu_div_internal div;
>> - struct ccu_internal mux;
>> - struct ccu_common common;
>> -};
>> -
>> -struct ccu_pll {
>> - struct ccu_common common;
>> -};
>> -
>> -#define TH_CCU_ARG(_shift, _width) \
>> - { \
>> - .shift = _shift, \
>> - .width = _width, \
>> - }
>> -
>> -#define TH_CCU_DIV_FLAGS(_shift, _width, _flags) \
>> - { \
>> - .shift = _shift, \
>> - .width = _width, \
>> - .flags = _flags, \
>> - }
>> -
>> -#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
>> - struct ccu_gate _struct = { \
>> - .enable = _gate, \
>> - .common = { \
>> - .clkid = _clkid, \
>> - .cfg0 = _reg, \
>> - .hw.init = CLK_HW_INIT_PARENTS_DATA( \
>> - _name, \
>> - _parent, \
>> - &clk_gate_ops, \
>> - _flags), \
>> - } \
>> - }
>> -
>> -static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>> -{
>> - return container_of(hw, struct ccu_common, hw);
>> -}
>> -
>> -static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
>> -{
>> - struct ccu_common *common = hw_to_ccu_common(hw);
>> -
>> - return container_of(common, struct ccu_mux, common);
>> -}
>> -
>> -static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
>> -{
>> - struct ccu_common *common = hw_to_ccu_common(hw);
>> +#include "clk-th1520.h"
>>
>> - return container_of(common, struct ccu_pll, common);
>> -}
>> -
>> -static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
>> -{
>> - struct ccu_common *common = hw_to_ccu_common(hw);
>> -
>> - return container_of(common, struct ccu_div, common);
>> -}
>> -
>> -static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
>> -{
>> - struct ccu_common *common = hw_to_ccu_common(hw);
>> -
>> - return container_of(common, struct ccu_gate, common);
>> -}
>> -
>> -static u8 ccu_get_parent_helper(struct ccu_common *common,
>> - struct ccu_internal *mux)
>> -{
>> - unsigned int val;
>> - u8 parent;
>> -
>> - regmap_read(common->map, common->cfg0, &val);
>> - parent = val >> mux->shift;
>> - parent &= GENMASK(mux->width - 1, 0);
>> -
>> - return parent;
>> -}
>> -
>> -static int ccu_set_parent_helper(struct ccu_common *common,
>> - struct ccu_internal *mux,
>> - u8 index)
>> -{
>> - return regmap_update_bits(common->map, common->cfg0,
>> - GENMASK(mux->width - 1, 0) << mux->shift,
>> - index << mux->shift);
>> -}
>> -
>> -static void ccu_disable_helper(struct ccu_common *common, u32 gate)
>> -{
>> - if (!gate)
>> - return;
>> - regmap_update_bits(common->map, common->cfg0,
>> - gate, ~gate);
>> -}
>> -
>> -static int ccu_enable_helper(struct ccu_common *common, u32 gate)
>> -{
>> - unsigned int val;
>> - int ret;
>> -
>> - if (!gate)
>> - return 0;
>> -
>> - ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
>> - regmap_read(common->map, common->cfg0, &val);
>> - return ret;
>> -}
>> -
>> -static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
>> -{
>> - unsigned int val;
>> -
>> - if (!gate)
>> - return true;
>> -
>> - regmap_read(common->map, common->cfg0, &val);
>> - return val & gate;
>> -}
>> -
>> -static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
>> - unsigned long parent_rate)
>> -{
>> - struct ccu_div *cd = hw_to_ccu_div(hw);
>> - unsigned long rate;
>> - unsigned int val;
>> -
>> - regmap_read(cd->common.map, cd->common.cfg0, &val);
>> - val = val >> cd->div.shift;
>> - val &= GENMASK(cd->div.width - 1, 0);
>> - rate = divider_recalc_rate(hw, parent_rate, val, NULL,
>> - cd->div.flags, cd->div.width);
>> -
>> - return rate;
>> -}
>> -
>> -static u8 ccu_div_get_parent(struct clk_hw *hw)
>> -{
>> - struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> - return ccu_get_parent_helper(&cd->common, &cd->mux);
>> -}
>> -
>> -static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
>> -{
>> - struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> - return ccu_set_parent_helper(&cd->common, &cd->mux, index);
>> -}
>> -
>> -static void ccu_div_disable(struct clk_hw *hw)
>> -{
>> - struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> - ccu_disable_helper(&cd->common, cd->enable);
>> -}
>> -
>> -static int ccu_div_enable(struct clk_hw *hw)
>> -{
>> - struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> - return ccu_enable_helper(&cd->common, cd->enable);
>> -}
>> -
>> -static int ccu_div_is_enabled(struct clk_hw *hw)
>> -{
>> - struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> - return ccu_is_enabled_helper(&cd->common, cd->enable);
>> -}
>> -
>> -static const struct clk_ops ccu_div_ops = {
>> - .disable = ccu_div_disable,
>> - .enable = ccu_div_enable,
>> - .is_enabled = ccu_div_is_enabled,
>> - .get_parent = ccu_div_get_parent,
>> - .set_parent = ccu_div_set_parent,
>> - .recalc_rate = ccu_div_recalc_rate,
>> - .determine_rate = clk_hw_determine_rate_no_reparent,
>> -};
>> -
>> -static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
>> - unsigned long parent_rate)
>> -{
>> - struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> - unsigned long div, mul, frac;
>> - unsigned int cfg0, cfg1;
>> - u64 rate = parent_rate;
>> -
>> - regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> - regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> -
>> - mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
>> - div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
>> - if (!(cfg1 & TH1520_PLL_DSMPD)) {
>> - mul <<= TH1520_PLL_FRAC_BITS;
>> - frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
>> - mul += frac;
>> - div <<= TH1520_PLL_FRAC_BITS;
>> - }
>> - rate = parent_rate * mul;
>> - rate = rate / div;
>> - return rate;
>> -}
>> -
>> -static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
>> - unsigned long parent_rate)
>> -{
>> - struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> - unsigned long div, rate = parent_rate;
>> - unsigned int cfg0, cfg1;
>> -
>> - regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> - regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> -
>> - if (cfg1 & TH1520_PLL_BYPASS)
>> - return rate;
>> -
>> - div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
>> - FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
>> -
>> - rate = rate / div;
>> -
>> - return rate;
>> -}
>> -
>> -static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
>> - unsigned long parent_rate)
>> -{
>> - unsigned long rate = parent_rate;
>> -
>> - rate = th1520_pll_vco_recalc_rate(hw, rate);
>> - rate = th1520_pll_postdiv_recalc_rate(hw, rate);
>> -
>> - return rate;
>> -}
>> -
>> -static const struct clk_ops clk_pll_ops = {
>> - .recalc_rate = ccu_pll_recalc_rate,
>> -};
>> +#define NR_CLKS (CLK_UART_SCLK + 1)
>>
>> static const struct clk_parent_data osc_24m_clk[] = {
>> { .index = 0 }
>> @@ -956,15 +668,6 @@ static struct ccu_common *th1520_gate_clks[] = {
>> &sram3_clk.common,
>> };
>>
>> -#define NR_CLKS (CLK_UART_SCLK + 1)
>
> Why did this move?
>
>> -
>> -static const struct regmap_config th1520_clk_regmap_config = {
>> - .reg_bits = 32,
>> - .val_bits = 32,
>> - .reg_stride = 4,
>> - .fast_io = true,
>> -};
>> -
>> static int th1520_clk_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> diff --git a/drivers/clk/thead/clk-th1520.c b/drivers/clk/thead/clk-th1520.c
>> new file mode 100644
>> index 000000000000..e2bfe56de9af
>> --- /dev/null
>> +++ b/drivers/clk/thead/clk-th1520.c
>> @@ -0,0 +1,188 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
>> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
>> + * Authors: Yangtao Li <frank.li@vivo.com>
>> + */
>> +
>> +#include "clk-th1520.h"
>
> Explicitly include linux headers here, don't just put them all into a
> header file. It helps us easily see what C files are using different
> parts of the kernel infrastructure.
>
>> +
>> +static u8 ccu_get_parent_helper(struct ccu_common *common,
>> + struct ccu_internal *mux)
>> +{
>> + unsigned int val;
>> + u8 parent;
>> +
>> + regmap_read(common->map, common->cfg0, &val);
>> + parent = val >> mux->shift;
>> + parent &= GENMASK(mux->width - 1, 0);
>> +
>> + return parent;
>> +}
>> +
>> +static int ccu_set_parent_helper(struct ccu_common *common,
>> + struct ccu_internal *mux, u8 index)
>> +{
>> + return regmap_update_bits(common->map, common->cfg0,
>> + GENMASK(mux->width - 1, 0) << mux->shift,
>> + index << mux->shift);
>> +}
>> +
>> +static void ccu_disable_helper(struct ccu_common *common, u32 gate)
>> +{
>> + if (!gate)
>> + return;
>> + regmap_update_bits(common->map, common->cfg0, gate, ~gate);
>> +}
>> +
>> +static int ccu_enable_helper(struct ccu_common *common, u32 gate)
>> +{
>> + unsigned int val;
>> + int ret;
>> +
>> + if (!gate)
>> + return 0;
>> +
>> + ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
>> + regmap_read(common->map, common->cfg0, &val);
>> + return ret;
>> +}
>> +
>> +static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
>> +{
>> + unsigned int val;
>> +
>> + if (!gate)
>> + return true;
>> +
>> + regmap_read(common->map, common->cfg0, &val);
>> + return val & gate;
>> +}
>> +
>> +static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + struct ccu_div *cd = hw_to_ccu_div(hw);
>> + unsigned long rate;
>> + unsigned int val;
>> +
>> + regmap_read(cd->common.map, cd->common.cfg0, &val);
>> + val = val >> cd->div.shift;
>> + val &= GENMASK(cd->div.width - 1, 0);
>> + rate = divider_recalc_rate(hw, parent_rate, val, NULL, cd->div.flags,
>> + cd->div.width);
>> +
>> + return rate;
>> +}
>> +
>> +static u8 ccu_div_get_parent(struct clk_hw *hw)
>> +{
>> + struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> + return ccu_get_parent_helper(&cd->common, &cd->mux);
>> +}
>> +
>> +static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> + struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> + return ccu_set_parent_helper(&cd->common, &cd->mux, index);
>> +}
>> +
>> +static void ccu_div_disable(struct clk_hw *hw)
>> +{
>> + struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> + ccu_disable_helper(&cd->common, cd->enable);
>> +}
>> +
>> +static int ccu_div_enable(struct clk_hw *hw)
>> +{
>> + struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> + return ccu_enable_helper(&cd->common, cd->enable);
>> +}
>> +
>> +static int ccu_div_is_enabled(struct clk_hw *hw)
>> +{
>> + struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> + return ccu_is_enabled_helper(&cd->common, cd->enable);
>> +}
>> +
>> +const struct clk_ops ccu_div_ops = {
>> + .disable = ccu_div_disable,
>> + .enable = ccu_div_enable,
>> + .is_enabled = ccu_div_is_enabled,
>> + .get_parent = ccu_div_get_parent,
>> + .set_parent = ccu_div_set_parent,
>> + .recalc_rate = ccu_div_recalc_rate,
>> + .determine_rate = clk_hw_determine_rate_no_reparent,
>> +};
>> +
>> +static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> + unsigned long div, mul, frac;
>> + unsigned int cfg0, cfg1;
>> + u64 rate = parent_rate;
>> +
>> + regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> + regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> +
>> + mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
>> + div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
>> + if (!(cfg1 & TH1520_PLL_DSMPD)) {
>> + mul <<= TH1520_PLL_FRAC_BITS;
>> + frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
>> + mul += frac;
>> + div <<= TH1520_PLL_FRAC_BITS;
>> + }
>> + rate = parent_rate * mul;
>> + rate = rate / div;
>> + return rate;
>> +}
>> +
>> +static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> + unsigned long div, rate = parent_rate;
>> + unsigned int cfg0, cfg1;
>> +
>> + regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> + regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> +
>> + if (cfg1 & TH1520_PLL_BYPASS)
>> + return rate;
>> +
>> + div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
>> + FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
>> +
>> + rate = rate / div;
>> +
>> + return rate;
>> +}
>> +
>> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + unsigned long rate = parent_rate;
>> +
>> + rate = th1520_pll_vco_recalc_rate(hw, rate);
>> + rate = th1520_pll_postdiv_recalc_rate(hw, rate);
>> +
>> + return rate;
>> +}
>> +
>> +const struct clk_ops clk_pll_ops = {
>> + .recalc_rate = ccu_pll_recalc_rate,
>> +};
>> +
>> +const struct regmap_config th1520_clk_regmap_config = {
>
> I don't get why this is moved.
>
>> + .reg_bits = 32,
>> + .val_bits = 32,
>> + .reg_stride = 4,
>> + .fast_io = true,
>> +};
>> diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
>> new file mode 100644
>> index 000000000000..285d41e65008
>> --- /dev/null
>> +++ b/drivers/clk/thead/clk-th1520.h
>> @@ -0,0 +1,134 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
>> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
>> + * Authors: Yangtao Li <frank.li@vivo.com>
>> + *
>> + * clk-th1520.h - Common definitions for T-HEAD TH1520 Clock Drivers
>> + */
>> +
>> +#ifndef CLK_TH1520_H
>> +#define CLK_TH1520_H
>> +
>> +#include <dt-bindings/clock/thead,th1520-clk-ap.h>
>
> dt-bindings comes after linux includes, but this shouldn't be here
> anyway.
>
>> +#include <linux/bitfield.h>
>> +#include <linux/clk-provider.h>
>
> Seems we have to have this one for clk_hw.
>
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>
> I don't see these includes used here, so remove them.
>
>> +#include <linux/regmap.h>
>
> Forward declare regmap and drop the include
>
> struct regmap;
>
>> +
>> +#define TH1520_PLL_POSTDIV2 GENMASK(26, 24)
>> +#define TH1520_PLL_POSTDIV1 GENMASK(22, 20)
>> +#define TH1520_PLL_FBDIV GENMASK(19, 8)
>> +#define TH1520_PLL_REFDIV GENMASK(5, 0)
>> +#define TH1520_PLL_BYPASS BIT(30)
>> +#define TH1520_PLL_DSMPD BIT(24)
>> +#define TH1520_PLL_FRAC GENMASK(23, 0)
>> +#define TH1520_PLL_FRAC_BITS 24
>
> Are these going to be used in multiple drivers?
>
>> +
>> +struct ccu_internal {
>> + u8 shift;
>> + u8 width;
>> +};
>> +
>> +struct ccu_div_internal {
>> + u8 shift;
>> + u8 width;
>> + u32 flags;
>> +};
>> +
>> +struct ccu_common {
>> + int clkid;
>> + struct regmap *map;
>> + u16 cfg0;
>> + u16 cfg1;
>> + struct clk_hw hw;
>> +};
>> +
>> +struct ccu_mux {
>> + struct ccu_internal mux;
>> + struct ccu_common common;
>> +};
>> +
>> +struct ccu_gate {
>> + u32 enable;
>> + struct ccu_common common;
>> +};
>> +
>> +struct ccu_div {
>> + u32 enable;
>> + struct ccu_div_internal div;
>> + struct ccu_internal mux;
>> + struct ccu_common common;
>> +};
>> +
>> +struct ccu_pll {
>> + struct ccu_common common;
>> +};
>> +
>> +#define TH_CCU_ARG(_shift, _width) \
>> + { \
>> + .shift = _shift, \
>> + .width = _width, \
>> + }
>> +
>> +#define TH_CCU_DIV_FLAGS(_shift, _width, _flags) \
>> + { \
>> + .shift = _shift, \
>> + .width = _width, \
>> + .flags = _flags, \
>> + }
>> +
>> +#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
>> + struct ccu_gate _struct = { \
>> + .enable = _gate, \
>> + .common = { \
>> + .clkid = _clkid, \
>> + .cfg0 = _reg, \
>> + .hw.init = CLK_HW_INIT_PARENTS_DATA( \
>> + _name, \
>> + _parent, \
>> + &clk_gate_ops, \
>> + _flags), \
>> + } \
>> + }
>> +
>> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>> +{
>> + return container_of(hw, struct ccu_common, hw);
>> +}
>> +
>> +static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
>> +{
>> + struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> + return container_of(common, struct ccu_mux, common);
>> +}
>> +
>> +static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
>> +{
>> + struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> + return container_of(common, struct ccu_pll, common);
>> +}
>> +
>> +static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
>> +{
>> + struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> + return container_of(common, struct ccu_div, common);
>> +}
>> +
>> +static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
>> +{
>> + struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> + return container_of(common, struct ccu_gate, common);
>> +}
>> +
>> +extern const struct clk_ops ccu_div_ops;
>> +extern const struct clk_ops clk_pll_ops;
>> +extern const struct regmap_config th1520_clk_regmap_config;
>
> Why is the regmap config exported?
The regmap_config is exported to allow reuse across multiple drivers.
Initially, I passed the clock VOSYS address space using the reg property
and created the regmap from it, enabling other drivers to utilize the
same configuration. Later, I switched to a regmap-based syscon approach
but haven’t moved the regmap_config back to the AP driver.
Based on Krzysztof's feedback, using the reg property to pass
memory-mapped registers is preferred. If needed, I can create a separate
regmap_config for the VO subsystem instead of reusing the existing one.
I will also address the other points you mentioned in your review.
Thank you for your feedback.
Michał
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code
2024-12-04 13:54 ` Michal Wilczynski
@ 2024-12-05 7:31 ` Krzysztof Kozlowski
0 siblings, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-05 7:31 UTC (permalink / raw)
To: Michal Wilczynski, Stephen Boyd, airlied, aou, conor+dt, drew,
frank.binns, guoren, jassisinghbrar, jszhang, krzk+dt,
m.szyprowski, maarten.lankhorst, matt.coster, mripard, mturquette,
palmer, paul.walmsley, robh, simona, tzimmermann, ulf.hansson,
wefu
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 04/12/2024 14:54, Michal Wilczynski wrote:
>
>
> On 12/3/24 20:56, Stephen Boyd wrote:
>> Quoting Michal Wilczynski (2024-12-03 05:41:24)
>>> diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
>>> index 7ee0bec1f251..d7cf88390b69 100644
>>> --- a/drivers/clk/thead/Makefile
>>> +++ b/drivers/clk/thead/Makefile
>>> @@ -1,2 +1,2 @@
>>> # SPDX-License-Identifier: GPL-2.0
>>> -obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o
>>> +obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
>>
>> Can the -ap driver be extended instead? Or are the clks in a different
>> IO region?
>
> The Video Output (VO) clocks reside in a different address space as
> defined in the T-HEAD manual 4.4.1 [1]. Therefore, creating a separate
> driver made sense to maintain clarity and adhere to the existing
There is no such rule, no convention even. But there is a rule and
convention of re-using drivers.
> convention of having one driver per subsystem, similar to the
You have here two drivers per subsystem, so even if there was such a
rule, you just broke it.
> AP-specific driver.
>
> [1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
>>
>>> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
>>> index 17e32ae08720..a6015805b859 100644
>>> --- a/drivers/clk/thead/clk-th1520-ap.c
>>> +++ b/drivers/clk/thead/clk-th1520-ap.c
>>> @@ -5,297 +5,9 @@
>>> * Authors: Yangtao Li <frank.li@vivo.com>
>>> */
>>>
>>> -#include <dt-bindings/clock/thead,th1520-clk-ap.h>
>>
>> Presumably this should stay here.
>>
>>> -#include <linux/bitfield.h>
>>> -#include <linux/clk-provider.h>
>>> -#include <linux/device.h>
>>> -#include <linux/module.h>
>>> -#include <linux/platform_device.h>
>>> -#include <linux/regmap.h>
>>
...
>>> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>>> +{
>>> + return container_of(hw, struct ccu_common, hw);
>>> +}
>>> +
>>> +static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
>>> +{
>>> + struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> + return container_of(common, struct ccu_mux, common);
>>> +}
>>> +
>>> +static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
>>> +{
>>> + struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> + return container_of(common, struct ccu_pll, common);
>>> +}
>>> +
>>> +static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
>>> +{
>>> + struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> + return container_of(common, struct ccu_div, common);
>>> +}
>>> +
>>> +static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
>>> +{
>>> + struct ccu_common *common = hw_to_ccu_common(hw);
>>> +
>>> + return container_of(common, struct ccu_gate, common);
>>> +}
>>> +
>>> +extern const struct clk_ops ccu_div_ops;
>>> +extern const struct clk_ops clk_pll_ops;
>>> +extern const struct regmap_config th1520_clk_regmap_config;
>>
>> Why is the regmap config exported?
>
> The regmap_config is exported to allow reuse across multiple drivers.
> Initially, I passed the clock VOSYS address space using the reg property
> and created the regmap from it, enabling other drivers to utilize the
> same configuration. Later, I switched to a regmap-based syscon approach
> but haven’t moved the regmap_config back to the AP driver.
It anyway in your original code cannot be exported. If you want to use
syscon, then use syscon, not exporting regmaps manually.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v1 02/14] dt-bindings: clock: thead,th1520: Rename header file
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
2024-12-03 13:41 ` [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 14:24 ` Rob Herring (Arm)
2024-12-03 15:41 ` Krzysztof Kozlowski
2024-12-03 13:41 ` [RFC PATCH v1 03/14] clk: thead: Enable clock gates with regmaps Michal Wilczynski
` (11 subsequent siblings)
13 siblings, 2 replies; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
As support for clocks from new subsystems is being added to the T-Head
TH1520 SoC, the header file name should reflect this broader scope. The
existing header file 'thead,th1520-clk-ap.h' includes the '-ap' suffix,
indicating it's specific to the Application Processor (AP) subsystem.
Rename the header file to 'thead,th1520-clk.h' to generalize it for all
subsystems. Update all references to this header file accordingly.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
.../devicetree/bindings/clock/thead,th1520-clk-ap.yaml | 4 ++--
.../devicetree/bindings/mailbox/thead,th1520-mbox.yaml | 2 +-
MAINTAINERS | 2 +-
arch/riscv/boot/dts/thead/th1520.dtsi | 2 +-
drivers/clk/thead/clk-th1520.h | 2 +-
.../clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h} | 0
6 files changed, 6 insertions(+), 6 deletions(-)
rename include/dt-bindings/clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h} (100%)
diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
index 0129bd0ba4b3..4a0806af2bf9 100644
--- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
@@ -32,7 +32,7 @@ properties:
"#clock-cells":
const: 1
description:
- See <dt-bindings/clock/thead,th1520-clk-ap.h> for valid indices.
+ See <dt-bindings/clock/thead,th1520-clk.h> for valid indices.
required:
- compatible
@@ -44,7 +44,7 @@ additionalProperties: false
examples:
- |
- #include <dt-bindings/clock/thead,th1520-clk-ap.h>
+ #include <dt-bindings/clock/thead,th1520-clk.h>
clock-controller@ef010000 {
compatible = "thead,th1520-clk-ap";
reg = <0xef010000 0x1000>;
diff --git a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
index 0971fb97896e..0b58b8d0d351 100644
--- a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
+++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
@@ -68,7 +68,7 @@ additionalProperties: false
examples:
- |
- #include <dt-bindings/clock/thead,th1520-clk-ap.h>
+ #include <dt-bindings/clock/thead,th1520-clk.h>
soc {
#address-cells = <2>;
#size-cells = <2>;
diff --git a/MAINTAINERS b/MAINTAINERS
index 7c85abf1dd1e..bd4bbf07d588 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20192,7 +20192,7 @@ F: drivers/clk/thead/
F: drivers/mailbox/mailbox-th1520.c
F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
F: drivers/pinctrl/pinctrl-th1520.c
-F: include/dt-bindings/clock/thead,th1520-clk-ap.h
+F: include/dt-bindings/clock/thead,th1520-clk.h
RNBD BLOCK DRIVERS
M: Md. Haris Iqbal <haris.iqbal@ionos.com>
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index acfe030e803a..dc2d554b4a71 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -5,7 +5,7 @@
*/
#include <dt-bindings/interrupt-controller/irq.h>
-#include <dt-bindings/clock/thead,th1520-clk-ap.h>
+#include <dt-bindings/clock/thead,th1520-clk.h>
/ {
compatible = "thead,th1520";
diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
index 285d41e65008..5d30f55e88a1 100644
--- a/drivers/clk/thead/clk-th1520.h
+++ b/drivers/clk/thead/clk-th1520.h
@@ -10,7 +10,7 @@
#ifndef CLK_TH1520_H
#define CLK_TH1520_H
-#include <dt-bindings/clock/thead,th1520-clk-ap.h>
+#include <dt-bindings/clock/thead,th1520-clk.h>
#include <linux/bitfield.h>
#include <linux/clk-provider.h>
#include <linux/device.h>
diff --git a/include/dt-bindings/clock/thead,th1520-clk-ap.h b/include/dt-bindings/clock/thead,th1520-clk.h
similarity index 100%
rename from include/dt-bindings/clock/thead,th1520-clk-ap.h
rename to include/dt-bindings/clock/thead,th1520-clk.h
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 02/14] dt-bindings: clock: thead,th1520: Rename header file
2024-12-03 13:41 ` [RFC PATCH v1 02/14] dt-bindings: clock: thead,th1520: Rename header file Michal Wilczynski
@ 2024-12-03 14:24 ` Rob Herring (Arm)
2024-12-03 15:41 ` Krzysztof Kozlowski
1 sibling, 0 replies; 38+ messages in thread
From: Rob Herring (Arm) @ 2024-12-03 14:24 UTC (permalink / raw)
To: Michal Wilczynski
Cc: mripard, m.szyprowski, maarten.lankhorst, mturquette, aou,
tzimmermann, frank.binns, simona, wefu, jszhang, linux-pm, drew,
guoren, palmer, linux-riscv, devicetree, paul.walmsley, linux-clk,
ulf.hansson, linux-kernel, krzk+dt, sboyd, dri-devel, matt.coster,
airlied, jassisinghbrar, conor+dt
On Tue, 03 Dec 2024 14:41:25 +0100, Michal Wilczynski wrote:
> As support for clocks from new subsystems is being added to the T-Head
> TH1520 SoC, the header file name should reflect this broader scope. The
> existing header file 'thead,th1520-clk-ap.h' includes the '-ap' suffix,
> indicating it's specific to the Application Processor (AP) subsystem.
>
> Rename the header file to 'thead,th1520-clk.h' to generalize it for all
> subsystems. Update all references to this header file accordingly.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> .../devicetree/bindings/clock/thead,th1520-clk-ap.yaml | 4 ++--
> .../devicetree/bindings/mailbox/thead,th1520-mbox.yaml | 2 +-
> MAINTAINERS | 2 +-
> arch/riscv/boot/dts/thead/th1520.dtsi | 2 +-
> drivers/clk/thead/clk-th1520.h | 2 +-
> .../clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h} | 0
> 6 files changed, 6 insertions(+), 6 deletions(-)
> rename include/dt-bindings/clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h} (100%)
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.example.dts:24:18: fatal error: dt-bindings/clock/thead,th1520-clk.h: No such file or directory
24 | #include <dt-bindings/clock/thead,th1520-clk.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.example.dts:18:18: fatal error: dt-bindings/clock/thead,th1520-clk.h: No such file or directory
18 | #include <dt-bindings/clock/thead,th1520-clk.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.example.dtb] Error 1
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241203134137.2114847-3-m.wilczynski@samsung.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 02/14] dt-bindings: clock: thead,th1520: Rename header file
2024-12-03 13:41 ` [RFC PATCH v1 02/14] dt-bindings: clock: thead,th1520: Rename header file Michal Wilczynski
2024-12-03 14:24 ` Rob Herring (Arm)
@ 2024-12-03 15:41 ` Krzysztof Kozlowski
1 sibling, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-03 15:41 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 03/12/2024 14:41, Michal Wilczynski wrote:
> As support for clocks from new subsystems is being added to the T-Head
> TH1520 SoC, the header file name should reflect this broader scope. The
No, there is no such rule, so "should" is not appropriate.
> existing header file 'thead,th1520-clk-ap.h' includes the '-ap' suffix,
> indicating it's specific to the Application Processor (AP) subsystem.
>
> Rename the header file to 'thead,th1520-clk.h' to generalize it for all
> subsystems. Update all references to this header file accordingly.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> .../devicetree/bindings/clock/thead,th1520-clk-ap.yaml | 4 ++--
> .../devicetree/bindings/mailbox/thead,th1520-mbox.yaml | 2 +-
> MAINTAINERS | 2 +-
> arch/riscv/boot/dts/thead/th1520.dtsi | 2 +-
> drivers/clk/thead/clk-th1520.h | 2 +-
> .../clock/{thead,th1520-clk-ap.h => thead,th1520-clk.h} | 0
Don't mix driver, bindings and DTS. See checkpatch warning. Don't rename
files just because you prefer other names.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v1 03/14] clk: thead: Enable clock gates with regmaps
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
2024-12-03 13:41 ` [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code Michal Wilczynski
2024-12-03 13:41 ` [RFC PATCH v1 02/14] dt-bindings: clock: thead,th1520: Rename header file Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 13:41 ` [RFC PATCH v1 04/14] clk: thead: Add clock driver for TH1520 Video Output subsystem Michal Wilczynski
` (10 subsequent siblings)
13 siblings, 0 replies; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
The current implementation of the CCU_GATE macro assumes direct access
to memory-mapped registers, which isn't suitable when using regmaps for
register access. In the TH1520 SoC, the address space for the VO (Video
Output) subsystem clocks is shared with other control registers, such as
those used for resetting the GPU. To prevent conflicts and ensure
synchronized access, it's important to access these registers via a
regmap.
This patch updates the CCU_GATE macro to support regmap-based access by
reusing the clk_ops from the divider clocks (ccu_div_ops). This change
allows the clock gates to be controlled through regmap, enabling proper
synchronization when multiple components interact with the shared
address space.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
drivers/clk/thead/clk-th1520.c | 10 ++++++++--
drivers/clk/thead/clk-th1520.h | 15 +++++++++++++++
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/thead/clk-th1520.c b/drivers/clk/thead/clk-th1520.c
index e2bfe56de9af..3ada8b98bd8e 100644
--- a/drivers/clk/thead/clk-th1520.c
+++ b/drivers/clk/thead/clk-th1520.c
@@ -120,8 +120,14 @@ const struct clk_ops ccu_div_ops = {
.determine_rate = clk_hw_determine_rate_no_reparent,
};
-static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
- unsigned long parent_rate)
+const struct clk_ops ccu_gate_ops = {
+ .disable = ccu_div_disable,
+ .enable = ccu_div_enable,
+ .is_enabled = ccu_div_is_enabled,
+};
+
+unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
{
struct ccu_pll *pll = hw_to_ccu_pll(hw);
unsigned long div, mul, frac;
diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
index 5d30f55e88a1..532afbbfea01 100644
--- a/drivers/clk/thead/clk-th1520.h
+++ b/drivers/clk/thead/clk-th1520.h
@@ -94,6 +94,20 @@ struct ccu_pll {
} \
}
+#define CCU_GATE_REGMAP(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
+ struct ccu_gate _struct = { \
+ .enable = _gate, \
+ .common = { \
+ .clkid = _clkid, \
+ .cfg0 = _reg, \
+ .hw.init = CLK_HW_INIT_PARENTS_DATA( \
+ _name, \
+ _parent, \
+ &ccu_gate_ops, \
+ _flags), \
+ } \
+ }
+
static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
{
return container_of(hw, struct ccu_common, hw);
@@ -130,5 +144,6 @@ static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
extern const struct clk_ops ccu_div_ops;
extern const struct clk_ops clk_pll_ops;
extern const struct regmap_config th1520_clk_regmap_config;
+extern const struct clk_ops ccu_gate_ops;
#endif /* CLK_TH1520_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [RFC PATCH v1 04/14] clk: thead: Add clock driver for TH1520 Video Output subsystem
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (2 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 03/14] clk: thead: Enable clock gates with regmaps Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 15:54 ` Krzysztof Kozlowski
2024-12-03 13:41 ` [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for " Michal Wilczynski
` (9 subsequent siblings)
13 siblings, 1 reply; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
The Video Output (VO) module on the T-Head TH1520 SoC has its own set of
clocks that need proper management. This commit introduces the
clk-th1520-vo driver to support the VO subsystem clocks.
Currently, only the clock gates are implemented, as they are the primary
relevant clocks for the VO subsystem at this stage.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
drivers/clk/thead/Kconfig | 11 ++
drivers/clk/thead/Makefile | 1 +
drivers/clk/thead/clk-th1520-vo.c | 168 +++++++++++++++++++
include/dt-bindings/clock/thead,th1520-clk.h | 34 ++++
4 files changed, 214 insertions(+)
create mode 100644 drivers/clk/thead/clk-th1520-vo.c
diff --git a/drivers/clk/thead/Kconfig b/drivers/clk/thead/Kconfig
index 95e0d9eb965e..937927a1a4b8 100644
--- a/drivers/clk/thead/Kconfig
+++ b/drivers/clk/thead/Kconfig
@@ -11,3 +11,14 @@ config CLK_THEAD_TH1520_AP
on the T-HEAD TH1520 SoC. This includes configuration of
both CPU PLLs, both DPU PLLs as well as the GMAC, VIDEO,
and TEE PLLs.
+
+config CLK_THEAD_TH1520_VO
+ bool "T-HEAD TH1520 VO clock support"
+ depends on ARCH_THEAD || COMPILE_TEST
+ depends on 64BIT
+ default ARCH_THEAD
+ select REGMAP_MMIO
+ help
+ Say yes here to support the VO sub system clock controller
+ on the T-HEAD TH1520 SoC. This includes clock gates for the
+ Video Output components like HDMI, MIPI, DPU and GPU.
diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
index d7cf88390b69..9afaee27b0b9 100644
--- a/drivers/clk/thead/Makefile
+++ b/drivers/clk/thead/Makefile
@@ -1,2 +1,3 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
+obj-$(CONFIG_CLK_THEAD_TH1520_VO) += clk-th1520.o clk-th1520-vo.o
diff --git a/drivers/clk/thead/clk-th1520-vo.c b/drivers/clk/thead/clk-th1520-vo.c
new file mode 100644
index 000000000000..3c6d246ab53a
--- /dev/null
+++ b/drivers/clk/thead/clk-th1520-vo.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2024 Samsung Electronics Co., Ltd.
+ * Author: Michal Wilczynski <m.wilczynski@samsung.com>
+ */
+
+#include <linux/mfd/syscon.h>
+#include "clk-th1520.h"
+
+#define NR_CLKS (CLK_HDMI_PIXCLK + 1)
+
+static const struct clk_parent_data video_pll_pd[] = {
+ /* TODO: provide a proper parent here */
+ NULL,
+};
+
+static CCU_GATE_REGMAP(CLK_AXI4_VO_ACLK, axi4_vo_aclk, "axi4-vo-aclk",
+ video_pll_pd, 0x50, BIT(0), 0);
+static CCU_GATE_REGMAP(CLK_GPU_CORE, gpu_core_clk, "gpu-core-clk", video_pll_pd,
+ 0x50, BIT(3), 0);
+static CCU_GATE_REGMAP(CLK_GPU_CFG_ACLK, gpu_cfg_aclk, "gpu-cfg-aclk",
+ video_pll_pd, 0x50, BIT(4), 0);
+static CCU_GATE_REGMAP(CLK_DPU_PIXELCLK0, dpu0_pixelclk, "dpu0-pixelclk",
+ video_pll_pd, 0x50, BIT(5), 0);
+static CCU_GATE_REGMAP(CLK_DPU_PIXELCLK1, dpu1_pixelclk, "dpu1-pixelclk",
+ video_pll_pd, 0x50, BIT(6), 0);
+static CCU_GATE_REGMAP(CLK_DPU_HCLK, dpu_hclk, "dpu-hclk", video_pll_pd, 0x50,
+ BIT(7), 0);
+static CCU_GATE_REGMAP(CLK_DPU_ACLK, dpu_aclk, "dpu-aclk", video_pll_pd, 0x50,
+ BIT(8), 0);
+static CCU_GATE_REGMAP(CLK_DPU_CCLK, dpu_cclk, "dpu-cclk", video_pll_pd, 0x50,
+ BIT(9), 0);
+static CCU_GATE_REGMAP(CLK_HDMI_SFR, hdmi_sfr_clk, "hdmi-sfr-clk", video_pll_pd,
+ 0x50, BIT(10), 0);
+static CCU_GATE_REGMAP(CLK_HDMI_PCLK, hdmi_pclk, "hdmi-pclk", video_pll_pd,
+ 0x50, BIT(11), 0);
+static CCU_GATE_REGMAP(CLK_HDMI_CEC, hdmi_cec_clk, "hdmi-cec-clk", video_pll_pd,
+ 0x50, BIT(12), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI0_PCLK, mipi_dsi0_pclk, "mipi-dsi0-pclk",
+ video_pll_pd, 0x50, BIT(13), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI1_PCLK, mipi_dsi1_pclk, "mipi-dsi1-pclk",
+ video_pll_pd, 0x50, BIT(14), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI0_CFG, mipi_dsi0_cfg_clk,
+ "mipi-dsi0-cfg-clk", video_pll_pd, 0x50, BIT(15), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI1_CFG, mipi_dsi1_cfg_clk,
+ "mipi-dsi1-cfg-clk", video_pll_pd, 0x50, BIT(16), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI0_REFCLK, mipi_dsi0_refclk,
+ "mipi-dsi0-refclk", video_pll_pd, 0x50, BIT(17), 0);
+static CCU_GATE_REGMAP(CLK_MIPI_DSI1_REFCLK, mipi_dsi1_refclk,
+ "mipi-dsi1-refclk", video_pll_pd, 0x50, BIT(18), 0);
+static CCU_GATE_REGMAP(CLK_HDMI_I2S, hdmi_i2c_clk, "hdmi-i2c-clk", video_pll_pd,
+ 0x50, BIT(19), 0);
+static CCU_GATE_REGMAP(CLK_X2H_DPU1_ACLK, x2h_dpu1_aclk, "x2h-dpu1-aclk",
+ video_pll_pd, 0x50, BIT(20), 0);
+static CCU_GATE_REGMAP(CLK_X2H_DPU_ACLK, x2h_dpu_aclk, "x2h-dpu-aclk",
+ video_pll_pd, 0x50, BIT(21), 0);
+static CCU_GATE_REGMAP(CLK_AXI4_VO_PCLK, axi4_vo_pclk, "axi4-vo-pclk",
+ video_pll_pd, 0x50, BIT(22), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_VOSYS_DPU_PCLK, iopmp_vosys_dpu_pclk,
+ "iopmp-vosys-dpu-pclk", video_pll_pd, 0x50, BIT(23), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_VOSYS_DPU1_PCLK, iopmp_vosys_dpu1_pclk,
+ "iopmp-vosys-dpu1-pclk", video_pll_pd, 0x50, BIT(24), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_VOSYS_GPU_PCLK, iopmp_vosys_gpu_pclk,
+ "iopmp-vosys-gpu-pclk", video_pll_pd, 0x50, BIT(25), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_DPU1_ACLK, iopmp_dpu1_aclk, "iopmp-dpu1-aclk",
+ video_pll_pd, 0x50, BIT(27), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_DPU_ACLK, iopmp_dpu_aclk, "iopmp-dpu-aclk",
+ video_pll_pd, 0x50, BIT(28), 0);
+static CCU_GATE_REGMAP(CLK_IOPMP_GPU_ACLK, iopmp_gpu_aclk, "iopmp-gpu-aclk",
+ video_pll_pd, 0x50, BIT(29), 0);
+static CCU_GATE_REGMAP(CLK_MIPIDSI0_PIXCLK, mipi_dsi0_pixclk,
+ "mipi-dsi0-pixclk", video_pll_pd, 0x50, BIT(30), 0);
+static CCU_GATE_REGMAP(CLK_MIPIDSI1_PIXCLK, mipi_dsi1_pixclk,
+ "mipi-dsi1-pixclk", video_pll_pd, 0x50, BIT(31), 0);
+static CCU_GATE_REGMAP(CLK_HDMI_PIXCLK, hdmi_pixclk, "hdmi-pixclk",
+ video_pll_pd, 0x54, BIT(0), 0);
+
+static struct ccu_common *th1520_vo_gate_clks[] = {
+ &axi4_vo_aclk.common,
+ &gpu_core_clk.common,
+ &gpu_cfg_aclk.common,
+ &dpu0_pixelclk.common,
+ &dpu1_pixelclk.common,
+ &dpu_hclk.common,
+ &dpu_aclk.common,
+ &dpu_cclk.common,
+ &hdmi_sfr_clk.common,
+ &hdmi_pclk.common,
+ &hdmi_cec_clk.common,
+ &mipi_dsi0_pclk.common,
+ &mipi_dsi1_pclk.common,
+ &mipi_dsi0_cfg_clk.common,
+ &mipi_dsi1_cfg_clk.common,
+ &mipi_dsi0_refclk.common,
+ &mipi_dsi1_refclk.common,
+ &hdmi_i2c_clk.common,
+ &x2h_dpu1_aclk.common,
+ &x2h_dpu_aclk.common,
+ &axi4_vo_pclk.common,
+ &iopmp_vosys_dpu_pclk.common,
+ &iopmp_vosys_dpu1_pclk.common,
+ &iopmp_vosys_gpu_pclk.common,
+ &iopmp_dpu1_aclk.common,
+ &iopmp_dpu_aclk.common,
+ &iopmp_gpu_aclk.common,
+ &mipi_dsi0_pixclk.common,
+ &mipi_dsi1_pixclk.common,
+ &hdmi_pixclk.common
+};
+
+static int th1520_clk_vo_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct clk_hw_onecell_data *priv;
+ struct regmap *map;
+ struct clk_hw *hw;
+ int ret, i;
+
+ priv = devm_kzalloc(dev, struct_size(priv, hws, NR_CLKS), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->num = NR_CLKS;
+
+ map = syscon_regmap_lookup_by_phandle(np, "thead,vosys-regmap");
+ if (IS_ERR(map))
+ return PTR_ERR(map);
+
+ for (i = 0; i < ARRAY_SIZE(th1520_vo_gate_clks); i++) {
+ struct ccu_gate *cg = hw_to_ccu_gate(&th1520_vo_gate_clks[i]->hw);
+
+ th1520_vo_gate_clks[i]->map = map;
+
+ ret = devm_clk_hw_register(dev, &th1520_vo_gate_clks[i]->hw);
+ if (ret)
+ return ret;
+
+ priv->hws[cg->common.clkid] = hw;
+ }
+
+ ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, priv);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct of_device_id th1520_clk_vo_match[] = {
+ {
+ .compatible = "thead,th1520-clk-vo",
+ },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, th1520_clk_vo_match);
+
+static struct platform_driver th1520_clk_vo_driver = {
+ .probe = th1520_clk_vo_probe,
+ .driver = {
+ .name = "th1520-clk-vo",
+ .of_match_table = th1520_clk_vo_match,
+ },
+};
+module_platform_driver(th1520_clk_vo_driver);
+
+MODULE_DESCRIPTION("T-HEAD TH1520 VO Clock driver");
+MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@samsung.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/dt-bindings/clock/thead,th1520-clk.h b/include/dt-bindings/clock/thead,th1520-clk.h
index a199784b3512..86a7cf2c9acf 100644
--- a/include/dt-bindings/clock/thead,th1520-clk.h
+++ b/include/dt-bindings/clock/thead,th1520-clk.h
@@ -7,6 +7,7 @@
#ifndef _DT_BINDINGS_CLK_TH1520_H_
#define _DT_BINDINGS_CLK_TH1520_H_
+/* AP clocks */
#define CLK_CPU_PLL0 0
#define CLK_CPU_PLL1 1
#define CLK_GMAC_PLL 2
@@ -93,4 +94,37 @@
#define CLK_SRAM3 83
#define CLK_PLL_GMAC_100M 84
#define CLK_UART_SCLK 85
+
+/* VO clocks */
+#define CLK_AXI4_VO_ACLK 0
+#define CLK_GPU_CORE 1
+#define CLK_GPU_CFG_ACLK 2
+#define CLK_DPU_PIXELCLK0 3
+#define CLK_DPU_PIXELCLK1 4
+#define CLK_DPU_HCLK 5
+#define CLK_DPU_ACLK 6
+#define CLK_DPU_CCLK 7
+#define CLK_HDMI_SFR 8
+#define CLK_HDMI_PCLK 9
+#define CLK_HDMI_CEC 10
+#define CLK_MIPI_DSI0_PCLK 11
+#define CLK_MIPI_DSI1_PCLK 12
+#define CLK_MIPI_DSI0_CFG 13
+#define CLK_MIPI_DSI1_CFG 14
+#define CLK_MIPI_DSI0_REFCLK 15
+#define CLK_MIPI_DSI1_REFCLK 16
+#define CLK_HDMI_I2S 17
+#define CLK_X2H_DPU1_ACLK 18
+#define CLK_X2H_DPU_ACLK 19
+#define CLK_AXI4_VO_PCLK 20
+#define CLK_IOPMP_VOSYS_DPU_PCLK 21
+#define CLK_IOPMP_VOSYS_DPU1_PCLK 22
+#define CLK_IOPMP_VOSYS_GPU_PCLK 23
+#define CLK_IOPMP_DPU1_ACLK 24
+#define CLK_IOPMP_DPU_ACLK 25
+#define CLK_IOPMP_GPU_ACLK 26
+#define CLK_MIPIDSI0_PIXCLK 27
+#define CLK_MIPIDSI1_PIXCLK 28
+#define CLK_HDMI_PIXCLK 29
+
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 04/14] clk: thead: Add clock driver for TH1520 Video Output subsystem
2024-12-03 13:41 ` [RFC PATCH v1 04/14] clk: thead: Add clock driver for TH1520 Video Output subsystem Michal Wilczynski
@ 2024-12-03 15:54 ` Krzysztof Kozlowski
0 siblings, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-03 15:54 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 03/12/2024 14:41, Michal Wilczynski wrote:
> The Video Output (VO) module on the T-Head TH1520 SoC has its own set of
> clocks that need proper management. This commit introduces the
> clk-th1520-vo driver to support the VO subsystem clocks.
>
> Currently, only the clock gates are implemented, as they are the primary
> relevant clocks for the VO subsystem at this stage.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> drivers/clk/thead/Kconfig | 11 ++
> drivers/clk/thead/Makefile | 1 +
> drivers/clk/thead/clk-th1520-vo.c | 168 +++++++++++++++++++
> include/dt-bindings/clock/thead,th1520-clk.h | 34 ++++
Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (3 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 04/14] clk: thead: Add clock driver for TH1520 Video Output subsystem Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 14:24 ` Rob Herring (Arm)
2024-12-03 15:45 ` Krzysztof Kozlowski
2024-12-03 13:41 ` [RFC PATCH v1 06/14] dt-bindings: clock: thead,th1520: Rename YAML schema file Michal Wilczynski
` (8 subsequent siblings)
13 siblings, 2 replies; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
The device tree bindings for the T-Head TH1520 SoC clocks currently
support only the Application Processor (AP) subsystem. This commit
extends the bindings to include the Video Output (VO) subsystem clocks.
Update the YAML schema to define the VO subsystem clocks, allowing the
clock driver to configure and manage these clocks appropriately. This
addition is necessary to enable the proper operation of the video output
features on the TH1520 SoC.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
.../bindings/clock/thead,th1520-clk-ap.yaml | 31 +++++++++++++++----
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
index 4a0806af2bf9..5a8f1041f766 100644
--- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
@@ -4,11 +4,13 @@
$id: http://devicetree.org/schemas/clock/thead,th1520-clk-ap.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: T-HEAD TH1520 AP sub-system clock controller
+title: T-HEAD TH1520 sub-systems clock controller
description: |
- The T-HEAD TH1520 AP sub-system clock controller configures the
- CPU, DPU, GMAC and TEE PLLs.
+ The T-HEAD TH1520 sub-systems clock controller configures the
+ CPU, DPU, GMAC and TEE PLLs for the AP subsystem. For the VO
+ subsystem clock gates can be configured for the HDMI, MIPI and
+ the GPU.
SoC reference manual
https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
@@ -20,7 +22,9 @@ maintainers:
properties:
compatible:
- const: thead,th1520-clk-ap
+ enum:
+ - thead,th1520-clk-ap
+ - thead,th1520-clk-vo
reg:
maxItems: 1
@@ -29,6 +33,17 @@ properties:
items:
- description: main oscillator (24MHz)
+ thead,vosys-regmap:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: |
+ Phandle to a syscon node representing the shared register
+ space of the VO (Video Output) subsystem. This register space
+ includes both clock control registers and other control
+ registers used for operations like resetting the GPU. Since
+ these registers reside in the same address space, access to
+ them is coordinated through a shared syscon regmap provided by
+ the specified syscon node.
+
"#clock-cells":
const: 1
description:
@@ -36,8 +51,6 @@ properties:
required:
- compatible
- - reg
- - clocks
- "#clock-cells"
additionalProperties: false
@@ -51,3 +64,9 @@ examples:
clocks = <&osc>;
#clock-cells = <1>;
};
+
+ clock-controller-vo {
+ compatible = "thead,th1520-clk-vo";
+ thead,vosys-regmap = <&vosys_regmap>;
+ #clock-cells = <1>;
+ };
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem
2024-12-03 13:41 ` [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for " Michal Wilczynski
@ 2024-12-03 14:24 ` Rob Herring (Arm)
2024-12-03 15:45 ` Krzysztof Kozlowski
1 sibling, 0 replies; 38+ messages in thread
From: Rob Herring (Arm) @ 2024-12-03 14:24 UTC (permalink / raw)
To: Michal Wilczynski
Cc: maarten.lankhorst, aou, wefu, jassisinghbrar, jszhang, mturquette,
dri-devel, devicetree, ulf.hansson, mripard, linux-kernel,
frank.binns, matt.coster, linux-riscv, linux-pm, guoren, sboyd,
linux-clk, m.szyprowski, drew, krzk+dt, palmer, paul.walmsley,
airlied, simona, tzimmermann, conor+dt
On Tue, 03 Dec 2024 14:41:28 +0100, Michal Wilczynski wrote:
> The device tree bindings for the T-Head TH1520 SoC clocks currently
> support only the Application Processor (AP) subsystem. This commit
> extends the bindings to include the Video Output (VO) subsystem clocks.
>
> Update the YAML schema to define the VO subsystem clocks, allowing the
> clock driver to configure and manage these clocks appropriately. This
> addition is necessary to enable the proper operation of the video output
> features on the TH1520 SoC.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> .../bindings/clock/thead,th1520-clk-ap.yaml | 31 +++++++++++++++----
> 1 file changed, 25 insertions(+), 6 deletions(-)
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241203134137.2114847-6-m.wilczynski@samsung.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem
2024-12-03 13:41 ` [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for " Michal Wilczynski
2024-12-03 14:24 ` Rob Herring (Arm)
@ 2024-12-03 15:45 ` Krzysztof Kozlowski
2024-12-04 10:11 ` Michal Wilczynski
1 sibling, 1 reply; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-03 15:45 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 03/12/2024 14:41, Michal Wilczynski wrote:
> The device tree bindings for the T-Head TH1520 SoC clocks currently
> support only the Application Processor (AP) subsystem. This commit
> extends the bindings to include the Video Output (VO) subsystem clocks.
>
> Update the YAML schema to define the VO subsystem clocks, allowing the
> clock driver to configure and manage these clocks appropriately. This
> addition is necessary to enable the proper operation of the video output
> features on the TH1520 SoC.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> .../bindings/clock/thead,th1520-clk-ap.yaml | 31 +++++++++++++++----
> 1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> index 4a0806af2bf9..5a8f1041f766 100644
> --- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> +++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> @@ -4,11 +4,13 @@
> $id: http://devicetree.org/schemas/clock/thead,th1520-clk-ap.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: T-HEAD TH1520 AP sub-system clock controller
> +title: T-HEAD TH1520 sub-systems clock controller
>
> description: |
> - The T-HEAD TH1520 AP sub-system clock controller configures the
> - CPU, DPU, GMAC and TEE PLLs.
> + The T-HEAD TH1520 sub-systems clock controller configures the
> + CPU, DPU, GMAC and TEE PLLs for the AP subsystem. For the VO
> + subsystem clock gates can be configured for the HDMI, MIPI and
> + the GPU.
>
> SoC reference manual
> https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
> @@ -20,7 +22,9 @@ maintainers:
>
> properties:
> compatible:
> - const: thead,th1520-clk-ap
> + enum:
> + - thead,th1520-clk-ap
> + - thead,th1520-clk-vo
>
> reg:
> maxItems: 1
> @@ -29,6 +33,17 @@ properties:
> items:
> - description: main oscillator (24MHz)
>
> + thead,vosys-regmap:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: |
> + Phandle to a syscon node representing the shared register
> + space of the VO (Video Output) subsystem. This register space
> + includes both clock control registers and other control
> + registers used for operations like resetting the GPU. Since
It seems you wanted to implement reset controller...
> + these registers reside in the same address space, access to
> + them is coordinated through a shared syscon regmap provided by
> + the specified syscon node.
Drop last sentence. syscon regmap is a Linux term, not hardware one.
Anyway, this needs to be constrained per variant.
> +
> "#clock-cells":
> const: 1
> description:
> @@ -36,8 +51,6 @@ properties:
>
> required:
> - compatible
> - - reg
No, that's a clear NAK. You claim you have no address space but in the
same time you have address space via regmap.
> - - clocks
Nope, not explained, unless you wanted to make it different per variants.
> - "#clock-cells"
>
> additionalProperties: false
> @@ -51,3 +64,9 @@ examples:
> clocks = <&osc>;
> #clock-cells = <1>;
> };
> +
> + clock-controller-vo {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "thead,th1520-clk-vo";
> + thead,vosys-regmap = <&vosys_regmap>;
That's a "reg" property. Do not encode address space as something else.
> + #clock-cells = <1>;
> + };
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem
2024-12-03 15:45 ` Krzysztof Kozlowski
@ 2024-12-04 10:11 ` Michal Wilczynski
2024-12-04 20:21 ` Stephen Boyd
2024-12-05 7:27 ` Krzysztof Kozlowski
0 siblings, 2 replies; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-04 10:11 UTC (permalink / raw)
To: Krzysztof Kozlowski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 12/3/24 16:45, Krzysztof Kozlowski wrote:
> On 03/12/2024 14:41, Michal Wilczynski wrote:
>> The device tree bindings for the T-Head TH1520 SoC clocks currently
>> support only the Application Processor (AP) subsystem. This commit
>> extends the bindings to include the Video Output (VO) subsystem clocks.
>>
>> Update the YAML schema to define the VO subsystem clocks, allowing the
>> clock driver to configure and manage these clocks appropriately. This
>> addition is necessary to enable the proper operation of the video output
>> features on the TH1520 SoC.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>> .../bindings/clock/thead,th1520-clk-ap.yaml | 31 +++++++++++++++----
>> 1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>> index 4a0806af2bf9..5a8f1041f766 100644
>> --- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>> +++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>> @@ -4,11 +4,13 @@
>> $id: https://protect2.fireeye.com/v1/url?k=f595e769-941ef222-f5946c26-74fe485fb305-6d0b73471bbfc1a2&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fclock%2Fthead%2Cth1520-clk-ap.yaml%23
>> $schema: https://protect2.fireeye.com/v1/url?k=5b94114b-3a1f0400-5b959a04-74fe485fb305-0e2c50f5c24cf3e9&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>
>> -title: T-HEAD TH1520 AP sub-system clock controller
>> +title: T-HEAD TH1520 sub-systems clock controller
>>
>> description: |
>> - The T-HEAD TH1520 AP sub-system clock controller configures the
>> - CPU, DPU, GMAC and TEE PLLs.
>> + The T-HEAD TH1520 sub-systems clock controller configures the
>> + CPU, DPU, GMAC and TEE PLLs for the AP subsystem. For the VO
>> + subsystem clock gates can be configured for the HDMI, MIPI and
>> + the GPU.
>>
>> SoC reference manual
>> https://protect2.fireeye.com/v1/url?k=cceb6120-ad60746b-cceaea6f-74fe485fb305-b294b70f1b52a5ab&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf
>> @@ -20,7 +22,9 @@ maintainers:
>>
>> properties:
>> compatible:
>> - const: thead,th1520-clk-ap
>> + enum:
>> + - thead,th1520-clk-ap
>> + - thead,th1520-clk-vo
>>
>> reg:
>> maxItems: 1
>> @@ -29,6 +33,17 @@ properties:
>> items:
>> - description: main oscillator (24MHz)
>>
>> + thead,vosys-regmap:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description: |
>> + Phandle to a syscon node representing the shared register
>> + space of the VO (Video Output) subsystem. This register space
>> + includes both clock control registers and other control
>> + registers used for operations like resetting the GPU. Since
>
>
> It seems you wanted to implement reset controller...
Thank you for your feedback.
I understand your concern about the reset controller. In the TH1520 SoC,
the GPU doesn't have its own reset controller. Instead, its reset is
managed through the power domain's registers as part of the power-up
sequence.
According to the Video Image Processing Manual 1.4.1 [1], the GPU
requires the following steps to power up:
1) Enable the GPU clock gate.
2) After 32 core clock cycles, release the GPU soft reset
Since these steps are closely tied to power management, I implemented
the reset functionality within the power-domain driver.
Because the GPU doesn't support the resets property, introducing a reset
controller wouldn't align with the existing device tree well.
[1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20Video%20Image%20Processing%20User%20Manual.pdf
>
>> + these registers reside in the same address space, access to
>> + them is coordinated through a shared syscon regmap provided by
>> + the specified syscon node.
>
> Drop last sentence. syscon regmap is a Linux term, not hardware one.
>
> Anyway, this needs to be constrained per variant.
>
>> +
>> "#clock-cells":
>> const: 1
>> description:
>> @@ -36,8 +51,6 @@ properties:
>>
>> required:
>> - compatible
>> - - reg
>
> No, that's a clear NAK. You claim you have no address space but in the
> same time you have address space via regmap.
I see your concern. The VOSYS subsystem's address space includes
registers for various components, such as clock gates and reset
controls, which are scattered throughout the address space as specified
in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
regmap for access, but I realize this might not be the best approach.
To address this, I'll specify the 'reg' property in each node to define
the address ranges explicitly fragmenting the address space for the VOSYS
manually.
vosys_clk: clock-controller@ffef528050 {
compatible = "thead,th1520-clk-vo";
reg = <0xff 0xef528050 0x0 0x8>;
#clock-cells = <1>;
};
pd: power-domain@ffef528000 {
compatible = "thead,th1520-pd";
reg = <0xff 0xef528000 0x0 0x8>;
#power-domain-cells = <1>;
};
I would be happy to proceed like this if that's okay.
[2] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
>
>> - - clocks
>
> Nope, not explained, unless you wanted to make it different per variants.
>
>> - "#clock-cells"
>>
>> additionalProperties: false
>> @@ -51,3 +64,9 @@ examples:
>> clocks = <&osc>;
>> #clock-cells = <1>;
>> };
>> +
>> + clock-controller-vo {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://protect2.fireeye.com/v1/url?k=1e9cac96-7f17b9dd-1e9d27d9-74fe485fb305-f0538482114df941&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=https%3A%2F%2Fdevicetree-specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-basics.html%23generic-names-recommendation
>
>
>> + compatible = "thead,th1520-clk-vo";
>> + thead,vosys-regmap = <&vosys_regmap>;
>
> That's a "reg" property. Do not encode address space as something else.
>
>
>> + #clock-cells = <1>;
>> + };
>
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem
2024-12-04 10:11 ` Michal Wilczynski
@ 2024-12-04 20:21 ` Stephen Boyd
2024-12-04 20:22 ` Stephen Boyd
2024-12-05 7:28 ` Krzysztof Kozlowski
2024-12-05 7:27 ` Krzysztof Kozlowski
1 sibling, 2 replies; 38+ messages in thread
From: Stephen Boyd @ 2024-12-04 20:21 UTC (permalink / raw)
To: Krzysztof Kozlowski, Michal Wilczynski, airlied, aou, conor+dt,
drew, frank.binns, guoren, jassisinghbrar, jszhang, krzk+dt,
m.szyprowski, maarten.lankhorst, matt.coster, mripard, mturquette,
palmer, paul.walmsley, robh, simona, tzimmermann, ulf.hansson,
wefu
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
Quoting Michal Wilczynski (2024-12-04 02:11:26)
> On 12/3/24 16:45, Krzysztof Kozlowski wrote:
> > On 03/12/2024 14:41, Michal Wilczynski wrote:
>
> [1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20Video%20Image%20Processing%20User%20Manual.pdf
> >
> >> + these registers reside in the same address space, access to
> >> + them is coordinated through a shared syscon regmap provided by
> >> + the specified syscon node.
> >
> > Drop last sentence. syscon regmap is a Linux term, not hardware one.
> >
> > Anyway, this needs to be constrained per variant.
> >
> >> +
> >> "#clock-cells":
> >> const: 1
> >> description:
> >> @@ -36,8 +51,6 @@ properties:
> >>
> >> required:
> >> - compatible
> >> - - reg
> >
> > No, that's a clear NAK. You claim you have no address space but in the
> > same time you have address space via regmap.
>
> I see your concern. The VOSYS subsystem's address space includes
> registers for various components, such as clock gates and reset
> controls, which are scattered throughout the address space as specified
> in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
> regmap for access, but I realize this might not be the best approach.
>
> To address this, I'll specify the 'reg' property in each node to define
> the address ranges explicitly fragmenting the address space for the VOSYS
> manually.
>
> vosys_clk: clock-controller@ffef528050 {
> compatible = "thead,th1520-clk-vo";
> reg = <0xff 0xef528050 0x0 0x8>;
> #clock-cells = <1>;
> };
>
> pd: power-domain@ffef528000 {
> compatible = "thead,th1520-pd";
> reg = <0xff 0xef528000 0x0 0x8>;
> #power-domain-cells = <1>;
> };
You should have one node:
clock-controller@ffef528000 {
compatible = "thead,th1520-vo";
reg = <0xff 0xef528050 0x0 0x1a04>;
#clock-cells = <1>;
#power-domain-cells = <1>;
};
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem
2024-12-04 20:21 ` Stephen Boyd
@ 2024-12-04 20:22 ` Stephen Boyd
2024-12-05 7:28 ` Krzysztof Kozlowski
1 sibling, 0 replies; 38+ messages in thread
From: Stephen Boyd @ 2024-12-04 20:22 UTC (permalink / raw)
To: Krzysztof Kozlowski, Michal Wilczynski, airlied, aou, conor+dt,
drew, frank.binns, guoren, jassisinghbrar, jszhang, krzk+dt,
m.szyprowski, maarten.lankhorst, matt.coster, mripard, mturquette,
palmer, paul.walmsley, robh, simona, tzimmermann, ulf.hansson,
wefu
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
Quoting Stephen Boyd (2024-12-04 12:21:11)
> Quoting Michal Wilczynski (2024-12-04 02:11:26)
> >
> > To address this, I'll specify the 'reg' property in each node to define
> > the address ranges explicitly fragmenting the address space for the VOSYS
> > manually.
> >
> > vosys_clk: clock-controller@ffef528050 {
> > compatible = "thead,th1520-clk-vo";
> > reg = <0xff 0xef528050 0x0 0x8>;
> > #clock-cells = <1>;
> > };
> >
> > pd: power-domain@ffef528000 {
> > compatible = "thead,th1520-pd";
> > reg = <0xff 0xef528000 0x0 0x8>;
> > #power-domain-cells = <1>;
> > };
>
> You should have one node:
>
> clock-controller@ffef528000 {
> compatible = "thead,th1520-vo";
> reg = <0xff 0xef528050 0x0 0x1a04>;
Apologies for the typo. Here's the correct line:
reg = <0xff 0xef528000 0x0 0x1a04>;
> #clock-cells = <1>;
> #power-domain-cells = <1>;
> };
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem
2024-12-04 20:21 ` Stephen Boyd
2024-12-04 20:22 ` Stephen Boyd
@ 2024-12-05 7:28 ` Krzysztof Kozlowski
1 sibling, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-05 7:28 UTC (permalink / raw)
To: Stephen Boyd, Michal Wilczynski, airlied, aou, conor+dt, drew,
frank.binns, guoren, jassisinghbrar, jszhang, krzk+dt,
m.szyprowski, maarten.lankhorst, matt.coster, mripard, mturquette,
palmer, paul.walmsley, robh, simona, tzimmermann, ulf.hansson,
wefu
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 04/12/2024 21:21, Stephen Boyd wrote:
>>> No, that's a clear NAK. You claim you have no address space but in the
>>> same time you have address space via regmap.
>>
>> I see your concern. The VOSYS subsystem's address space includes
>> registers for various components, such as clock gates and reset
>> controls, which are scattered throughout the address space as specified
>> in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
>> regmap for access, but I realize this might not be the best approach.
>>
>> To address this, I'll specify the 'reg' property in each node to define
>> the address ranges explicitly fragmenting the address space for the VOSYS
>> manually.
>>
>> vosys_clk: clock-controller@ffef528050 {
>> compatible = "thead,th1520-clk-vo";
>> reg = <0xff 0xef528050 0x0 0x8>;
>> #clock-cells = <1>;
>> };
>>
>> pd: power-domain@ffef528000 {
>> compatible = "thead,th1520-pd";
>> reg = <0xff 0xef528000 0x0 0x8>;
>> #power-domain-cells = <1>;
>> };
>
> You should have one node:
>
> clock-controller@ffef528000 {
> compatible = "thead,th1520-vo";
> reg = <0xff 0xef528050 0x0 0x1a04>;
Yes, obviously, but probably entire block:
<0xff 0xef528000 0x0 0x1000>;
> #clock-cells = <1>;
> #power-domain-cells = <1>;
> };
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem
2024-12-04 10:11 ` Michal Wilczynski
2024-12-04 20:21 ` Stephen Boyd
@ 2024-12-05 7:27 ` Krzysztof Kozlowski
1 sibling, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-05 7:27 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 04/12/2024 11:11, Michal Wilczynski wrote:
>
>
> On 12/3/24 16:45, Krzysztof Kozlowski wrote:
>> On 03/12/2024 14:41, Michal Wilczynski wrote:
>>> The device tree bindings for the T-Head TH1520 SoC clocks currently
>>> support only the Application Processor (AP) subsystem. This commit
>>> extends the bindings to include the Video Output (VO) subsystem clocks.
>>>
>>> Update the YAML schema to define the VO subsystem clocks, allowing the
>>> clock driver to configure and manage these clocks appropriately. This
>>> addition is necessary to enable the proper operation of the video output
>>> features on the TH1520 SoC.
>>>
>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>>> ---
>>> .../bindings/clock/thead,th1520-clk-ap.yaml | 31 +++++++++++++++----
>>> 1 file changed, 25 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>>> index 4a0806af2bf9..5a8f1041f766 100644
>>> --- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>>> @@ -4,11 +4,13 @@
>>> $id: https://protect2.fireeye.com/v1/url?k=f595e769-941ef222-f5946c26-74fe485fb305-6d0b73471bbfc1a2&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fclock%2Fthead%2Cth1520-clk-ap.yaml%23
>>> $schema: https://protect2.fireeye.com/v1/url?k=5b94114b-3a1f0400-5b959a04-74fe485fb305-0e2c50f5c24cf3e9&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>>
>>> -title: T-HEAD TH1520 AP sub-system clock controller
>>> +title: T-HEAD TH1520 sub-systems clock controller
>>>
>>> description: |
>>> - The T-HEAD TH1520 AP sub-system clock controller configures the
>>> - CPU, DPU, GMAC and TEE PLLs.
>>> + The T-HEAD TH1520 sub-systems clock controller configures the
>>> + CPU, DPU, GMAC and TEE PLLs for the AP subsystem. For the VO
>>> + subsystem clock gates can be configured for the HDMI, MIPI and
>>> + the GPU.
>>>
>>> SoC reference manual
>>> https://protect2.fireeye.com/v1/url?k=cceb6120-ad60746b-cceaea6f-74fe485fb305-b294b70f1b52a5ab&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf
>>> @@ -20,7 +22,9 @@ maintainers:
>>>
>>> properties:
>>> compatible:
>>> - const: thead,th1520-clk-ap
>>> + enum:
>>> + - thead,th1520-clk-ap
>>> + - thead,th1520-clk-vo
>>>
>>> reg:
>>> maxItems: 1
>>> @@ -29,6 +33,17 @@ properties:
>>> items:
>>> - description: main oscillator (24MHz)
>>>
>>> + thead,vosys-regmap:
>>> + $ref: /schemas/types.yaml#/definitions/phandle
>>> + description: |
>>> + Phandle to a syscon node representing the shared register
>>> + space of the VO (Video Output) subsystem. This register space
>>> + includes both clock control registers and other control
>>> + registers used for operations like resetting the GPU. Since
>>
>>
>> It seems you wanted to implement reset controller...
>
> Thank you for your feedback.
>
> I understand your concern about the reset controller. In the TH1520 SoC,
> the GPU doesn't have its own reset controller. Instead, its reset is
> managed through the power domain's registers as part of the power-up
> sequence.
>
> According to the Video Image Processing Manual 1.4.1 [1], the GPU
> requires the following steps to power up:
>
> 1) Enable the GPU clock gate.
> 2) After 32 core clock cycles, release the GPU soft reset
>
> Since these steps are closely tied to power management, I implemented
> the reset functionality within the power-domain driver.
>
> Because the GPU doesn't support the resets property, introducing a reset
> controller wouldn't align with the existing device tree well.
So add resets to GPU. You said here that VO has reset registers, so it
should be a reset controller.
>
> [1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20Video%20Image%20Processing%20User%20Manual.pdf
>>
>>> + these registers reside in the same address space, access to
>>> + them is coordinated through a shared syscon regmap provided by
>>> + the specified syscon node.
>>
>> Drop last sentence. syscon regmap is a Linux term, not hardware one.
>>
>> Anyway, this needs to be constrained per variant.
>>
>>> +
>>> "#clock-cells":
>>> const: 1
>>> description:
>>> @@ -36,8 +51,6 @@ properties:
>>>
>>> required:
>>> - compatible
>>> - - reg
>>
>> No, that's a clear NAK. You claim you have no address space but in the
>> same time you have address space via regmap.
>
> I see your concern. The VOSYS subsystem's address space includes
> registers for various components, such as clock gates and reset
> controls, which are scattered throughout the address space as specified
> in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
> regmap for access, but I realize this might not be the best approach.
No, you miss the point of how device nodes are supposed to look like.
Don't bring your driver architecture here.
You cannot have regmap/syscon if you do not have reg. Your VOSYS is a
clock, reset and whatever-provider. Your power domain - if it does not
have reg - just does not exist. There is no such device and we do not
care if you have such device DRIVER.
>
> To address this, I'll specify the 'reg' property in each node to define
> the address ranges explicitly fragmenting the address space for the VOSYS
> manually.
>
> vosys_clk: clock-controller@ffef528050 {
> compatible = "thead,th1520-clk-vo";
> reg = <0xff 0xef528050 0x0 0x8>;
> #clock-cells = <1>;
> };
>
> pd: power-domain@ffef528000 {
> compatible = "thead,th1520-pd";
> reg = <0xff 0xef528000 0x0 0x8>;
> #power-domain-cells = <1>;
> };
I don't think you really get the point here. Clock controllers and power
domains per one clock or domain are also a no-go.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v1 06/14] dt-bindings: clock: thead,th1520: Rename YAML schema file
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (4 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for " Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 14:25 ` Rob Herring (Arm)
2024-12-03 15:45 ` Krzysztof Kozlowski
2024-12-03 13:41 ` [RFC PATCH v1 07/14] soc: thead: power-domain: Add skeleton power-domain driver for TH1520 Michal Wilczynski
` (7 subsequent siblings)
13 siblings, 2 replies; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
As support for clocks from new subsystems is being added to the T-Head
TH1520 SoC, the Device Tree binding YAML schema file name should reflect
this broader scope. The existing schema file 'thead,th1520-clk-ap.yaml'
includes the '-ap' suffix, indicating it's specific to the Application
Processor (AP) subsystem.
Rename the YAML schema file to 'thead,th1520-clk.yaml' to generalize it
for all subsystems. Update all references to this schema file
accordingly.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
.../clock/{thead,th1520-clk-ap.yaml => thead,th1520-clk.yaml} | 2 +-
MAINTAINERS | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
rename Documentation/devicetree/bindings/clock/{thead,th1520-clk-ap.yaml => thead,th1520-clk.yaml} (96%)
diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
similarity index 96%
rename from Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
rename to Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
index 5a8f1041f766..416c8882942e 100644
--- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
%YAML 1.2
---
-$id: http://devicetree.org/schemas/clock/thead,th1520-clk-ap.yaml#
+$id: http://devicetree.org/schemas/clock/thead,th1520-clk.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
title: T-HEAD TH1520 sub-systems clock controller
diff --git a/MAINTAINERS b/MAINTAINERS
index bd4bbf07d588..2f8f529e6a31 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20183,7 +20183,7 @@ M: Fu Wei <wefu@redhat.com>
L: linux-riscv@lists.infradead.org
S: Maintained
T: git https://github.com/pdp7/linux.git
-F: Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+F: Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
F: Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
F: Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
F: Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 06/14] dt-bindings: clock: thead,th1520: Rename YAML schema file
2024-12-03 13:41 ` [RFC PATCH v1 06/14] dt-bindings: clock: thead,th1520: Rename YAML schema file Michal Wilczynski
@ 2024-12-03 14:25 ` Rob Herring (Arm)
2024-12-03 15:45 ` Krzysztof Kozlowski
1 sibling, 0 replies; 38+ messages in thread
From: Rob Herring (Arm) @ 2024-12-03 14:25 UTC (permalink / raw)
To: Michal Wilczynski
Cc: mripard, frank.binns, aou, jszhang, sboyd, guoren, m.szyprowski,
dri-devel, maarten.lankhorst, wefu, jassisinghbrar, simona,
conor+dt, devicetree, mturquette, linux-kernel, linux-clk,
krzk+dt, ulf.hansson, palmer, paul.walmsley, linux-riscv,
tzimmermann, drew, airlied, matt.coster, linux-pm
On Tue, 03 Dec 2024 14:41:29 +0100, Michal Wilczynski wrote:
> As support for clocks from new subsystems is being added to the T-Head
> TH1520 SoC, the Device Tree binding YAML schema file name should reflect
> this broader scope. The existing schema file 'thead,th1520-clk-ap.yaml'
> includes the '-ap' suffix, indicating it's specific to the Application
> Processor (AP) subsystem.
>
> Rename the YAML schema file to 'thead,th1520-clk.yaml' to generalize it
> for all subsystems. Update all references to this schema file
> accordingly.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> .../clock/{thead,th1520-clk-ap.yaml => thead,th1520-clk.yaml} | 2 +-
> MAINTAINERS | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> rename Documentation/devicetree/bindings/clock/{thead,th1520-clk-ap.yaml => thead,th1520-clk.yaml} (96%)
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241203134137.2114847-7-m.wilczynski@samsung.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 06/14] dt-bindings: clock: thead,th1520: Rename YAML schema file
2024-12-03 13:41 ` [RFC PATCH v1 06/14] dt-bindings: clock: thead,th1520: Rename YAML schema file Michal Wilczynski
2024-12-03 14:25 ` Rob Herring (Arm)
@ 2024-12-03 15:45 ` Krzysztof Kozlowski
1 sibling, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-03 15:45 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 03/12/2024 14:41, Michal Wilczynski wrote:
> As support for clocks from new subsystems is being added to the T-Head
> TH1520 SoC, the Device Tree binding YAML schema file name should reflect
> this broader scope. The existing schema file 'thead,th1520-clk-ap.yaml'
> includes the '-ap' suffix, indicating it's specific to the Application
> Processor (AP) subsystem.
>
> Rename the YAML schema file to 'thead,th1520-clk.yaml' to generalize it
> for all subsystems. Update all references to this schema file
> accordingly.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> .../clock/{thead,th1520-clk-ap.yaml => thead,th1520-clk.yaml} | 2 +-
> MAINTAINERS | 2 +-
NAK, don't rename just because you added one more compatible (and anyway
never a separate patch).
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v1 07/14] soc: thead: power-domain: Add skeleton power-domain driver for TH1520
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (5 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 06/14] dt-bindings: clock: thead,th1520: Rename YAML schema file Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 15:58 ` Krzysztof Kozlowski
2024-12-03 13:41 ` [RFC PATCH v1 08/14] dt-bindings: power: thead,th1520: Add support for power domains Michal Wilczynski
` (6 subsequent siblings)
13 siblings, 1 reply; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
The T-Head TH1520 SoC contains multiple power islands that can be
programmatically turned on and off using the AON (Always-On) protocol
and a hardware mailbox [1]. The relevant mailbox driver has already been
merged into the mainline kernel in commit 5d4d263e1c6b ("mailbox:
Introduce support for T-head TH1520 Mailbox driver"); however, the AON
implementation is still under development.
This commit introduces a skeleton power-domain driver for the TH1520
SoC, designed to be easily extended to work with the AON protocol in the
future. Currently, it only supports the GPU. Since there is no
mechanism yet to turn the GPU power island on, the driver will only set
the relevant registers to bring the GPU out of the reset state. This
should be done after the power-up sequence requested through the mailbox
is completed.
Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf [1]
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
MAINTAINERS | 2 +
drivers/pmdomain/Kconfig | 1 +
drivers/pmdomain/Makefile | 1 +
drivers/pmdomain/thead/Kconfig | 12 ++
drivers/pmdomain/thead/Makefile | 2 +
drivers/pmdomain/thead/th1520-pm-domains.c | 195 ++++++++++++++++++
.../dt-bindings/power/thead,th1520-power.h | 19 ++
7 files changed, 232 insertions(+)
create mode 100644 drivers/pmdomain/thead/Kconfig
create mode 100644 drivers/pmdomain/thead/Makefile
create mode 100644 drivers/pmdomain/thead/th1520-pm-domains.c
create mode 100644 include/dt-bindings/power/thead,th1520-power.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 2f8f529e6a31..16fb58aa74b1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20192,7 +20192,9 @@ F: drivers/clk/thead/
F: drivers/mailbox/mailbox-th1520.c
F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
F: drivers/pinctrl/pinctrl-th1520.c
+F: drivers/pmdomain/thead/th1520-pm-domains.c
F: include/dt-bindings/clock/thead,th1520-clk.h
+F: include/dt-bindings/power/thead,th1520-power.h
RNBD BLOCK DRIVERS
M: Md. Haris Iqbal <haris.iqbal@ionos.com>
diff --git a/drivers/pmdomain/Kconfig b/drivers/pmdomain/Kconfig
index 23c64851a5b0..91f04ace35d4 100644
--- a/drivers/pmdomain/Kconfig
+++ b/drivers/pmdomain/Kconfig
@@ -16,6 +16,7 @@ source "drivers/pmdomain/st/Kconfig"
source "drivers/pmdomain/starfive/Kconfig"
source "drivers/pmdomain/sunxi/Kconfig"
source "drivers/pmdomain/tegra/Kconfig"
+source "drivers/pmdomain/thead/Kconfig"
source "drivers/pmdomain/ti/Kconfig"
source "drivers/pmdomain/xilinx/Kconfig"
diff --git a/drivers/pmdomain/Makefile b/drivers/pmdomain/Makefile
index a68ece2f4c68..7030f44a49df 100644
--- a/drivers/pmdomain/Makefile
+++ b/drivers/pmdomain/Makefile
@@ -14,6 +14,7 @@ obj-y += st/
obj-y += starfive/
obj-y += sunxi/
obj-y += tegra/
+obj-y += thead/
obj-y += ti/
obj-y += xilinx/
obj-y += core.o governor.o
diff --git a/drivers/pmdomain/thead/Kconfig b/drivers/pmdomain/thead/Kconfig
new file mode 100644
index 000000000000..8a063b3b96f3
--- /dev/null
+++ b/drivers/pmdomain/thead/Kconfig
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config TH1520_PM_DOMAINS
+ bool "Support TH1520 Power Domains"
+ depends on ARCH_THEAD || COMPILE_TEST
+ select REGMAP_MMIO
+ help
+ This driver enables power domain management for the T-HEAD
+ TH-1520 SoC. On this SoC there are number of power domains,
+ which can be managed independently. For example GPU, AUDIO,
+ NPU, DPU reside in their own power domains which can be
+ turned on/off.
diff --git a/drivers/pmdomain/thead/Makefile b/drivers/pmdomain/thead/Makefile
new file mode 100644
index 000000000000..adfdf5479c68
--- /dev/null
+++ b/drivers/pmdomain/thead/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_TH1520_PM_DOMAINS) += th1520-pm-domains.o
diff --git a/drivers/pmdomain/thead/th1520-pm-domains.c b/drivers/pmdomain/thead/th1520-pm-domains.c
new file mode 100644
index 000000000000..60bdd011b017
--- /dev/null
+++ b/drivers/pmdomain/thead/th1520-pm-domains.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Alibaba Group Holding Limited.
+ * Copyright (c) 2024 Samsung Electronics Co., Ltd.
+ * Author: Michal Wilczynski <m.wilczynski@samsung.com>
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_domain.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/power/thead,th1520-power.h>
+
+/* register offset in VOSYS_REGMAP */
+#define TH1520_GPU_RST_CFG 0x0
+#define TH1520_GPU_RST_CFG_MASK GENMASK(2, 0)
+
+/* register values */
+#define TH1520_GPU_SW_GPU_RST BIT(0)
+#define TH1520_GPU_SW_CLKGEN_RST BIT(1)
+
+struct th1520_power_domain {
+ struct regmap *reg;
+ struct generic_pm_domain genpd;
+ u32 rsrc;
+};
+
+struct th1520_power_info {
+ char *name;
+ u32 rsrc;
+};
+
+static const struct th1520_power_info th1520_pd_ranges[] = {
+ { "gpu", TH1520_AON_GPU_PD }
+};
+
+static inline struct th1520_power_domain *
+to_th1520_power_domain(struct generic_pm_domain *genpd)
+{
+ return container_of(genpd, struct th1520_power_domain, genpd);
+}
+
+static void th1520_rst_gpu_enable(struct regmap *reg)
+{
+ int val;
+
+ /* if the GPU is not in a reset state it, put it into one */
+ regmap_read(reg, TH1520_GPU_RST_CFG, &val);
+ if (val) {
+ regmap_update_bits(reg, TH1520_GPU_RST_CFG,
+ TH1520_GPU_RST_CFG_MASK, 0x0);
+ }
+
+ /* rst gpu clkgen */
+ regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_CLKGEN_RST);
+ /* rst gpu */
+ regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_GPU_RST);
+}
+
+static void th1520_rst_gpu_disable(struct regmap *reg)
+{
+ regmap_update_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_RST_CFG_MASK, 0x0);
+}
+
+static int th1520_pd_power_on(struct generic_pm_domain *domain)
+{
+ struct th1520_power_domain *pd = to_th1520_power_domain(domain);
+
+ /* The missing component here is the call to E902 core through the
+ * AON protocol using hardware mailbox.
+ */
+
+ /* Initially after the power up the GPU and GPU clocks are
+ * in the reset state. Get them from that state to normal operation.
+ */
+ th1520_rst_gpu_enable(pd->reg);
+
+ return 0;
+}
+
+static int th1520_pd_power_off(struct generic_pm_domain *domain)
+{
+ struct th1520_power_domain *pd = to_th1520_power_domain(domain);
+
+ /* The missing component here is the call to E902 core through the
+ * AON protocol using hardware mailbox.
+ */
+
+ /* Put the GPU into reset state after powering it off */
+ th1520_rst_gpu_disable(pd->reg);
+
+ return 0;
+}
+
+static struct generic_pm_domain *th1520_pd_xlate(const struct of_phandle_args *spec,
+ void *data)
+{
+ struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
+ struct genpd_onecell_data *pd_data = data;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
+ struct th1520_power_domain *pd;
+
+ pd = to_th1520_power_domain(pd_data->domains[i]);
+ if (pd->rsrc == spec->args[0]) {
+ domain = &pd->genpd;
+ break;
+ }
+ }
+
+ return domain;
+}
+
+static struct th1520_power_domain *
+th1520_add_pm_domain(struct device *dev, const struct th1520_power_info *pi)
+{
+ struct th1520_power_domain *pd;
+ int ret;
+
+ pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ return ERR_PTR(-ENOMEM);
+
+ pd->rsrc = pi->rsrc;
+ pd->genpd.power_on = th1520_pd_power_on;
+ pd->genpd.power_off = th1520_pd_power_off;
+ pd->genpd.name = pi->name;
+
+ ret = pm_genpd_init(&pd->genpd, NULL, true);
+ if (ret) {
+ devm_kfree(dev, pd);
+ return ERR_PTR(ret);
+ }
+
+ return pd;
+}
+
+static int th1520_pd_probe(struct platform_device *pdev)
+{
+ struct generic_pm_domain **domains;
+ struct genpd_onecell_data *pd_data;
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct regmap *reg;
+ int i;
+
+ reg = syscon_regmap_lookup_by_phandle(np, "thead,vosys-regmap");
+ if (IS_ERR(reg))
+ return PTR_ERR(reg);
+
+ domains = devm_kcalloc(dev, ARRAY_SIZE(th1520_pd_ranges),
+ sizeof(*domains), GFP_KERNEL);
+ if (!domains)
+ return -ENOMEM;
+
+ pd_data = devm_kzalloc(dev, sizeof(*pd_data), GFP_KERNEL);
+ if (!pd_data)
+ return -ENOMEM;
+
+ for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
+ struct th1520_power_domain *pd;
+
+ pd = th1520_add_pm_domain(dev, &th1520_pd_ranges[i]);
+ if (IS_ERR_OR_NULL(pd))
+ continue;
+
+ pd->reg = reg;
+ domains[i] = &pd->genpd;
+ dev_dbg(dev, "added power domain %s\n", pd->genpd.name);
+ }
+
+ pd_data->domains = domains;
+ pd_data->num_domains = ARRAY_SIZE(th1520_pd_ranges);
+ pd_data->xlate = th1520_pd_xlate;
+
+ return of_genpd_add_provider_onecell(dev->of_node, pd_data);
+}
+
+static const struct of_device_id th1520_pd_match[] = {
+ { .compatible = "thead,th1520-pd",},
+ { /* sentinel */ }
+};
+
+static struct platform_driver th1520_pd_driver = {
+ .driver = {
+ .name = "th1520-pd",
+ .of_match_table = th1520_pd_match,
+ },
+ .probe = th1520_pd_probe,
+};
+builtin_platform_driver(th1520_pd_driver);
diff --git a/include/dt-bindings/power/thead,th1520-power.h b/include/dt-bindings/power/thead,th1520-power.h
new file mode 100644
index 000000000000..30fb4e9892e7
--- /dev/null
+++ b/include/dt-bindings/power/thead,th1520-power.h
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2022 Alibaba Group Holding Limited.
+ * Copyright (c) 2024 Samsung Electronics Co., Ltd.
+ * Author: Michal Wilczynski <m.wilczynski@samsung.com>
+ */
+
+#ifndef __DT_BINDINGS_POWER_TH1520_H
+#define __DT_BINDINGS_POWER_TH1520_H
+
+#define TH1520_AON_AUDIO_PD 0
+#define TH1520_AON_VDEC_PD 1
+#define TH1520_AON_NPU_PD 2
+#define TH1520_AON_VENC_PD 3
+#define TH1520_AON_GPU_PD 4
+#define TH1520_AON_DSP0_PD 5
+#define TH1520_AON_DSP1_PD 6
+
+#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 07/14] soc: thead: power-domain: Add skeleton power-domain driver for TH1520
2024-12-03 13:41 ` [RFC PATCH v1 07/14] soc: thead: power-domain: Add skeleton power-domain driver for TH1520 Michal Wilczynski
@ 2024-12-03 15:58 ` Krzysztof Kozlowski
0 siblings, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-03 15:58 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 03/12/2024 14:41, Michal Wilczynski wrote:
> The T-Head TH1520 SoC contains multiple power islands that can be
> programmatically turned on and off using the AON (Always-On) protocol
> and a hardware mailbox [1]. The relevant mailbox driver has already been
> merged into the mainline kernel in commit 5d4d263e1c6b ("mailbox:
> Introduce support for T-head TH1520 Mailbox driver"); however, the AON
> implementation is still under development.
>
> This commit introduces a skeleton power-domain driver for the TH1520
> SoC, designed to be easily extended to work with the AON protocol in the
> future. Currently, it only supports the GPU. Since there is no
> mechanism yet to turn the GPU power island on, the driver will only set
> the relevant registers to bring the GPU out of the reset state. This
> should be done after the power-up sequence requested through the mailbox
> is completed.
>
> Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf [1]
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> MAINTAINERS | 2 +
> drivers/pmdomain/Kconfig | 1 +
> drivers/pmdomain/Makefile | 1 +
> drivers/pmdomain/thead/Kconfig | 12 ++
> drivers/pmdomain/thead/Makefile | 2 +
> drivers/pmdomain/thead/th1520-pm-domains.c | 195 ++++++++++++++++++
> .../dt-bindings/power/thead,th1520-power.h | 19 ++
> 7 files changed, 232 insertions(+)
Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
> create mode 100644 drivers/pmdomain/thead/Kconfig
> create mode 100644 drivers/pmdomain/thead/Makefile
> create mode 100644 drivers/pmdomain/thead/th1520-pm-domains.c
> create mode 100644 include/dt-bindings/power/thead,th1520-power.h
>
...
> +
> +static int th1520_pd_power_off(struct generic_pm_domain *domain)
> +{
> + struct th1520_power_domain *pd = to_th1520_power_domain(domain);
> +
> + /* The missing component here is the call to E902 core through the
Use Linux coding style comments (see coding style). This applies to
multiple places in your code.
> + * AON protocol using hardware mailbox.
> + */
> +
> + /* Put the GPU into reset state after powering it off */
> + th1520_rst_gpu_disable(pd->reg);
> +
> + return 0;
> +}
> +
> +static struct generic_pm_domain *th1520_pd_xlate(const struct of_phandle_args *spec,
> + void *data)
> +{
> + struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
> + struct genpd_onecell_data *pd_data = data;
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
> + struct th1520_power_domain *pd;
> +
> + pd = to_th1520_power_domain(pd_data->domains[i]);
> + if (pd->rsrc == spec->args[0]) {
> + domain = &pd->genpd;
> + break;
> + }
> + }
> +
> + return domain;
> +}
> +
> +static struct th1520_power_domain *
> +th1520_add_pm_domain(struct device *dev, const struct th1520_power_info *pi)
> +{
> + struct th1520_power_domain *pd;
> + int ret;
> +
> + pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + return ERR_PTR(-ENOMEM);
> +
> + pd->rsrc = pi->rsrc;
> + pd->genpd.power_on = th1520_pd_power_on;
> + pd->genpd.power_off = th1520_pd_power_off;
> + pd->genpd.name = pi->name;
> +
> + ret = pm_genpd_init(&pd->genpd, NULL, true);
> + if (ret) {
> + devm_kfree(dev, pd);
You should rather fail the probe. Failures of power domains are important.
> + return ERR_PTR(ret);
> + }
> +
> + return pd;
> +}
> +
> +static int th1520_pd_probe(struct platform_device *pdev)
> +{
> + struct generic_pm_domain **domains;
> + struct genpd_onecell_data *pd_data;
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct regmap *reg;
> + int i;
> +
> + reg = syscon_regmap_lookup_by_phandle(np, "thead,vosys-regmap");
> + if (IS_ERR(reg))
> + return PTR_ERR(reg);
> +
> + domains = devm_kcalloc(dev, ARRAY_SIZE(th1520_pd_ranges),
> + sizeof(*domains), GFP_KERNEL);
> + if (!domains)
> + return -ENOMEM;
> +
> + pd_data = devm_kzalloc(dev, sizeof(*pd_data), GFP_KERNEL);
> + if (!pd_data)
> + return -ENOMEM;
> +
> + for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
> + struct th1520_power_domain *pd;
> +
> + pd = th1520_add_pm_domain(dev, &th1520_pd_ranges[i]);
> + if (IS_ERR_OR_NULL(pd))
> + continue;
> +
> + pd->reg = reg;
> + domains[i] = &pd->genpd;
> + dev_dbg(dev, "added power domain %s\n", pd->genpd.name);
> + }
> +
> + pd_data->domains = domains;
> + pd_data->num_domains = ARRAY_SIZE(th1520_pd_ranges);
> + pd_data->xlate = th1520_pd_xlate;
> +
> + return of_genpd_add_provider_onecell(dev->of_node, pd_data);
> +}
> +
> +static const struct of_device_id th1520_pd_match[] = {
> + { .compatible = "thead,th1520-pd",},
> + { /* sentinel */ }
> +};
> +
Make the driver tristate and module. There is nothing here which
prevents it being a module.
> +builtin_platform_driver(th1520_pd_driver);
> diff --git a/include/dt-bindings/power/thead,th1520-power.h b/include/dt-bindings/power/thead,th1520-power.h
> new file mode 100644
> index 000000000000..30fb4e9892e7
> --- /dev/null
> +++ b/include/dt-bindings/power/thead,th1520-power.h
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0+
Wrong license. See checkpatch.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v1 08/14] dt-bindings: power: thead,th1520: Add support for power domains
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (6 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 07/14] soc: thead: power-domain: Add skeleton power-domain driver for TH1520 Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 15:25 ` Rob Herring (Arm)
2024-12-03 15:48 ` Krzysztof Kozlowski
2024-12-03 13:41 ` [RFC PATCH v1 09/14] riscv: Enable PM_GENERIC_DOMAINS for T-Head SoCs Michal Wilczynski
` (5 subsequent siblings)
13 siblings, 2 replies; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
Add power domain support to the Thead TH1520 clock controller bindings.
This enables devices to specify their power domain dependencies,
improving power management for components like the GPU.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
.../bindings/power/thead,th1520-power.yaml | 52 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 53 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/thead,th1520-power.yaml
diff --git a/Documentation/devicetree/bindings/power/thead,th1520-power.yaml b/Documentation/devicetree/bindings/power/thead,th1520-power.yaml
new file mode 100644
index 000000000000..528af54f4ca6
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/thead,th1520-power.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/thead,th1520-power.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: T-HEAD TH1520 Power Domain Controller
+
+maintainers:
+ - Michal Wilczynski <m.wilczynski@samsung.com>
+
+description: |
+ The T-HEAD TH1520 SoC includes a power domain controller responsible for
+ managing the power states of various hardware domains such as the GPU.
+
+ This binding describes the power domain controller node, which can be used by
+ devices to manage their power domains.
+
+properties:
+ compatible:
+ const: "thead,th1520-pd"
+
+ thead,vosys-regmap:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: |
+ Phandle to a syscon node representing the shared register space of the VO (Video Output) subsystem.
+ This register space includes both clock control registers and other control registers used for
+ operations like resetting the GPU. Since these registers reside in the same address space,
+ access to them is coordinated through a shared syscon regmap provided by the specified syscon node.
+
+ '#power-domain-cells':
+ const: 1
+
+required:
+ - compatible
+ - thead,vosys-regmap
+ - '#power-domain-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ vosys_regmap: vosys@ffef528000 {
+ compatible = "syscon";
+ reg = <0xff 0xef528000 0x0 0x1000>;
+ };
+
+ power-controller {
+ compatible = "thead,th1520-pd";
+ thead,vosys-regmap = <&vosys_regmap>;
+ #power-domain-cells = <1>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 16fb58aa74b1..acbe311087ad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20187,6 +20187,7 @@ F: Documentation/devicetree/bindings/clock/thead,th1520-clk.yaml
F: Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
F: Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
F: Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
+F: Documentation/devicetree/bindings/power/thead,th1520-power.yaml
F: arch/riscv/boot/dts/thead/
F: drivers/clk/thead/
F: drivers/mailbox/mailbox-th1520.c
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 08/14] dt-bindings: power: thead,th1520: Add support for power domains
2024-12-03 13:41 ` [RFC PATCH v1 08/14] dt-bindings: power: thead,th1520: Add support for power domains Michal Wilczynski
@ 2024-12-03 15:25 ` Rob Herring (Arm)
2024-12-03 15:48 ` Krzysztof Kozlowski
1 sibling, 0 replies; 38+ messages in thread
From: Rob Herring (Arm) @ 2024-12-03 15:25 UTC (permalink / raw)
To: Michal Wilczynski
Cc: m.szyprowski, frank.binns, linux-kernel, maarten.lankhorst,
paul.walmsley, jszhang, linux-riscv, ulf.hansson, mturquette,
wefu, conor+dt, dri-devel, linux-pm, airlied, aou, matt.coster,
guoren, devicetree, linux-clk, sboyd, simona, drew, krzk+dt,
jassisinghbrar, mripard, palmer, tzimmermann
On Tue, 03 Dec 2024 14:41:31 +0100, Michal Wilczynski wrote:
> Add power domain support to the Thead TH1520 clock controller bindings.
> This enables devices to specify their power domain dependencies,
> improving power management for components like the GPU.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> .../bindings/power/thead,th1520-power.yaml | 52 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 53 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/thead,th1520-power.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/thead,th1520-power.example.dtb: vosys@ffef528000: compatible: ['syscon'] is too short
from schema $id: http://devicetree.org/schemas/mfd/syscon-common.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/thead,th1520-power.example.dtb: vosys@ffef528000: reg: [[255, 4015161344], [0, 4096]] is too long
from schema $id: http://devicetree.org/schemas/mfd/syscon-common.yaml#
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241203134137.2114847-9-m.wilczynski@samsung.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [RFC PATCH v1 08/14] dt-bindings: power: thead,th1520: Add support for power domains
2024-12-03 13:41 ` [RFC PATCH v1 08/14] dt-bindings: power: thead,th1520: Add support for power domains Michal Wilczynski
2024-12-03 15:25 ` Rob Herring (Arm)
@ 2024-12-03 15:48 ` Krzysztof Kozlowski
1 sibling, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-03 15:48 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 03/12/2024 14:41, Michal Wilczynski wrote:
> +
> +title: T-HEAD TH1520 Power Domain Controller
> +
> +maintainers:
> + - Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +description: |
> + The T-HEAD TH1520 SoC includes a power domain controller responsible for
> + managing the power states of various hardware domains such as the GPU.
> +
> + This binding describes the power domain controller node, which can be used by
Do not describe the binding. Describe the hardware. Entire paragraph
feels pointless.
> + devices to manage their power domains.
> +
> +properties:
> + compatible:
> + const: "thead,th1520-pd"
You never tested the code you sent. Drop quotes. Limited review follows.
> +
> + thead,vosys-regmap:
NAK.
'reg' is for this.
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: |
> + Phandle to a syscon node representing the shared register space of the VO (Video Output) subsystem.
Please wrap code according to coding style (checkpatch is not a coding
style description, but only a tool).
> + This register space includes both clock control registers and other control registers used for
> + operations like resetting the GPU. Since these registers reside in the same address space,
> + access to them is coordinated through a shared syscon regmap provided by the specified syscon node.
> +
> + '#power-domain-cells':
> + const: 1
> +
> +required:
> + - compatible
> + - thead,vosys-regmap
> + - '#power-domain-cells'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + vosys_regmap: vosys@ffef528000 {
> + compatible = "syscon";
> + reg = <0xff 0xef528000 0x0 0x1000>;
> + };
Drop, not related.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v1 09/14] riscv: Enable PM_GENERIC_DOMAINS for T-Head SoCs
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (7 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 08/14] dt-bindings: power: thead,th1520: Add support for power domains Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 13:41 ` [RFC PATCH v1 10/14] drm/imagination: Add support for IMG BXM-4-64 GPU Michal Wilczynski
` (4 subsequent siblings)
13 siblings, 0 replies; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
T-Head SoCs feature separate power domains (power islands) for major
components like the GPU, Audio, and NPU. To manage the power states of
these components effectively, the kernel requires generic power domain
support.
This commit enables `CONFIG_PM_GENERIC_DOMAINS` for T-Head SoCs,
allowing the power domain driver for these components to be compiled and
integrated. This ensures proper power management and energy efficiency
on T-Head platforms.
By selecting `PM_GENERIC_DOMAINS`, we provide the necessary framework
for the power domain drivers to function correctly on RISC-V
architecture with T-Head SoCs.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
arch/riscv/Kconfig.socs | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index f51bb24bc84c..c414dc618b66 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -48,6 +48,7 @@ config ARCH_THEAD
bool "T-HEAD RISC-V SoCs"
depends on MMU && !XIP_KERNEL
select ERRATA_THEAD
+ select PM_GENERIC_DOMAINS if PM
help
This enables support for the RISC-V based T-HEAD SoCs.
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [RFC PATCH v1 10/14] drm/imagination: Add support for IMG BXM-4-64 GPU
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (8 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 09/14] riscv: Enable PM_GENERIC_DOMAINS for T-Head SoCs Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 15:49 ` Krzysztof Kozlowski
2024-12-03 13:41 ` [RFC PATCH v1 11/14] drm/imagination: Enable PowerVR driver for RISC-V Michal Wilczynski
` (3 subsequent siblings)
13 siblings, 1 reply; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
The IMG BXM-4-64 GPU is integrated into the T-Head TH1520 SoC. This
commit adds the compatible string "img,img-bxm-4-64" to the device tree
match table in the drm/imagination driver, enabling support for this
GPU.
By including this GPU in the compatible devices list, the driver can
initialize and manage the BXM-4-64 GPU on the TH1520 SoC, providing
graphics acceleration capabilities upstream.
This commit doesn't touch the img,powervr-rogue.yaml on purpose, as the
new dt-bindings schema was proposed [1], but not merged yet.
Link: https://lore.kernel.org/all/20241118-sets-bxs-4-64-patch-v1-v2-1-3fd45d9fb0cf@imgtec.com/ [1]
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
drivers/gpu/drm/imagination/pvr_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index 85ee9abd1811..8633a3a315b7 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -1475,6 +1475,7 @@ static void pvr_remove(struct platform_device *plat_dev)
static const struct of_device_id dt_match[] = {
{ .compatible = "img,img-axe", .data = NULL },
+ { .compatible = "img,img-bxm-4-64", .data = NULL },
{}
};
MODULE_DEVICE_TABLE(of, dt_match);
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 10/14] drm/imagination: Add support for IMG BXM-4-64 GPU
2024-12-03 13:41 ` [RFC PATCH v1 10/14] drm/imagination: Add support for IMG BXM-4-64 GPU Michal Wilczynski
@ 2024-12-03 15:49 ` Krzysztof Kozlowski
0 siblings, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-03 15:49 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 03/12/2024 14:41, Michal Wilczynski wrote:
> The IMG BXM-4-64 GPU is integrated into the T-Head TH1520 SoC. This
> commit adds the compatible string "img,img-bxm-4-64" to the device tree
> match table in the drm/imagination driver, enabling support for this
> GPU.
>
> By including this GPU in the compatible devices list, the driver can
> initialize and manage the BXM-4-64 GPU on the TH1520 SoC, providing
> graphics acceleration capabilities upstream.
>
> This commit doesn't touch the img,powervr-rogue.yaml on purpose, as the
> new dt-bindings schema was proposed [1], but not merged yet.
That's not related to the commit. This commit *cannot ever* touch the
bindings.
>
> Link: https://lore.kernel.org/all/20241118-sets-bxs-4-64-patch-v1-v2-1-3fd45d9fb0cf@imgtec.com/ [1]
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> drivers/gpu/drm/imagination/pvr_drv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index 85ee9abd1811..8633a3a315b7 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1475,6 +1475,7 @@ static void pvr_remove(struct platform_device *plat_dev)
>
> static const struct of_device_id dt_match[] = {
> { .compatible = "img,img-axe", .data = NULL },
> + { .compatible = "img,img-bxm-4-64", .data = NULL },
Undocumented compatible. Combine relevant patches into one patchsets, so
we see entire picture.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v1 11/14] drm/imagination: Enable PowerVR driver for RISC-V
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (9 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 10/14] drm/imagination: Add support for IMG BXM-4-64 GPU Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 13:41 ` [RFC PATCH v1 12/14] riscv: dts: Add Video Output clock and syscon regmap nodes Michal Wilczynski
` (2 subsequent siblings)
13 siblings, 0 replies; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
Several RISC-V boards feature Imagination GPUs that are compatible with
the PowerVR driver. An example is the IMG BXM-4-64 GPU on the Lichee Pi
4A board. This commit adjusts the driver's Kconfig dependencies to allow
the PowerVR driver to be compiled on the RISC-V architecture.
By enabling compilation on RISC-V, we expand support for these GPUs,
providing graphics acceleration capabilities and enhancing hardware
compatibility on RISC-V platforms.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
drivers/gpu/drm/imagination/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
index 3bfa2ac212dc..5f218896114c 100644
--- a/drivers/gpu/drm/imagination/Kconfig
+++ b/drivers/gpu/drm/imagination/Kconfig
@@ -3,7 +3,7 @@
config DRM_POWERVR
tristate "Imagination Technologies PowerVR (Series 6 and later) & IMG Graphics"
- depends on ARM64
+ depends on (ARM64 || RISCV)
depends on DRM
depends on PM
select DRM_EXEC
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [RFC PATCH v1 12/14] riscv: dts: Add Video Output clock and syscon regmap nodes
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (10 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 11/14] drm/imagination: Enable PowerVR driver for RISC-V Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 15:50 ` Krzysztof Kozlowski
2024-12-03 13:41 ` [RFC PATCH v1 13/14] riscv: dts: Introduce power domain node with simple-bus compatible Michal Wilczynski
2024-12-03 13:41 ` [RFC PATCH v1 14/14] riscv: dts: Add GPU node to TH1520 device tree Michal Wilczynski
13 siblings, 1 reply; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
The address space controlling the Video Output (VO) subsystem clocks
also contains control registers for GPU resets. To properly synchronize
access to this shared address space, create a syscon Device Tree node
for the VO registers and reference it in the clock controller node.
This change ensures coordinated access to the VO registers between the
clock controller and other drivers, preventing conflicts and maintaining
system stability.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index dc2d554b4a71..39d39059160d 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -489,6 +489,18 @@ clk: clock-controller@ffef010000 {
#clock-cells = <1>;
};
+ vosys_clk: clock-controller {
+ compatible = "thead,th1520-clk-vo";
+ thead,vosys-regmap = <&vosys_reg>;
+ #clock-cells = <1>;
+ };
+
+ vosys_reg: vosys@ffef528000 {
+ compatible = "thead,th1520-vosys", "syscon";
+ reg = <0xff 0xef528000 0x0 0x1000>;
+ status = "okay";
+ };
+
dmac0: dma-controller@ffefc00000 {
compatible = "snps,axi-dma-1.01a";
reg = <0xff 0xefc00000 0x0 0x1000>;
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 12/14] riscv: dts: Add Video Output clock and syscon regmap nodes
2024-12-03 13:41 ` [RFC PATCH v1 12/14] riscv: dts: Add Video Output clock and syscon regmap nodes Michal Wilczynski
@ 2024-12-03 15:50 ` Krzysztof Kozlowski
0 siblings, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-03 15:50 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 03/12/2024 14:41, Michal Wilczynski wrote:
> The address space controlling the Video Output (VO) subsystem clocks
> also contains control registers for GPU resets. To properly synchronize
> access to this shared address space, create a syscon Device Tree node
> for the VO registers and reference it in the clock controller node.
>
> This change ensures coordinated access to the VO registers between the
> clock controller and other drivers, preventing conflicts and maintaining
> system stability.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index dc2d554b4a71..39d39059160d 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -489,6 +489,18 @@ clk: clock-controller@ffef010000 {
> #clock-cells = <1>;
> };
>
> + vosys_clk: clock-controller {
Missing address space. You cannot have here nodes without unit address.
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
> + compatible = "thead,th1520-clk-vo";
> + thead,vosys-regmap = <&vosys_reg>;
> + #clock-cells = <1>;
> + };
> +
> + vosys_reg: vosys@ffef528000 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "thead,th1520-vosys", "syscon";
> + reg = <0xff 0xef528000 0x0 0x1000>;
> + status = "okay";
Where is it disabled? Drop.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v1 13/14] riscv: dts: Introduce power domain node with simple-bus compatible
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (11 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 12/14] riscv: dts: Add Video Output clock and syscon regmap nodes Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 15:52 ` Krzysztof Kozlowski
2024-12-03 13:41 ` [RFC PATCH v1 14/14] riscv: dts: Add GPU node to TH1520 device tree Michal Wilczynski
13 siblings, 1 reply; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
The DRM Imagination GPU requires a power-domain driver, but the driver
for "thead,th1520-aon" is not yet available. To ensure that the 'aon'
node and its child 'pd' node are properly recognized and probed by the
kernel, add "simple-bus" to the compatible property of the 'aon' node.
This change allows the kernel to treat the 'aon' node as a simple bus,
enabling the child nodes to be probed and initialized independently. It
ensures that the power domain can be managed appropriately until the
specific AON driver is developed.
This commit introduces some errors while running dtbs_check, as the aon
doesn't have the dt-bindings yet.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 39d39059160d..58f93ad3eb6e 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -6,6 +6,7 @@
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/clock/thead,th1520-clk.h>
+#include <dt-bindings/power/thead,th1520-power.h>
/ {
compatible = "thead,th1520";
@@ -229,6 +230,16 @@ stmmac_axi_config: stmmac-axi-config {
snps,blen = <0 0 64 32 0 0 0>;
};
+ aon {
+ compatible = "thead,th1520-aon", "simple-bus";
+
+ pd: power-domain {
+ compatible = "thead,th1520-pd";
+ thead,vosys-regmap = <&vosys_reg>;
+ #power-domain-cells = <1>;
+ };
+ };
+
soc {
compatible = "simple-bus";
interrupt-parent = <&plic>;
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 13/14] riscv: dts: Introduce power domain node with simple-bus compatible
2024-12-03 13:41 ` [RFC PATCH v1 13/14] riscv: dts: Introduce power domain node with simple-bus compatible Michal Wilczynski
@ 2024-12-03 15:52 ` Krzysztof Kozlowski
2024-12-04 10:34 ` Michal Wilczynski
0 siblings, 1 reply; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-03 15:52 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 03/12/2024 14:41, Michal Wilczynski wrote:
> The DRM Imagination GPU requires a power-domain driver, but the driver
> for "thead,th1520-aon" is not yet available. To ensure that the 'aon'
> node and its child 'pd' node are properly recognized and probed by the
> kernel, add "simple-bus" to the compatible property of the 'aon' node.
>
> This change allows the kernel to treat the 'aon' node as a simple bus,
> enabling the child nodes to be probed and initialized independently. It
> ensures that the power domain can be managed appropriately until the
> specific AON driver is developed.
>
> This commit introduces some errors while running dtbs_check, as the aon
> doesn't have the dt-bindings yet.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 39d39059160d..58f93ad3eb6e 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -6,6 +6,7 @@
>
> #include <dt-bindings/interrupt-controller/irq.h>
> #include <dt-bindings/clock/thead,th1520-clk.h>
> +#include <dt-bindings/power/thead,th1520-power.h>
>
> / {
> compatible = "thead,th1520";
> @@ -229,6 +230,16 @@ stmmac_axi_config: stmmac-axi-config {
> snps,blen = <0 0 64 32 0 0 0>;
> };
>
> + aon {
> + compatible = "thead,th1520-aon", "simple-bus";
1. No, that's not a bus.
2. Please run scripts/checkpatch.pl and fix reported warnings. Then
please run `scripts/checkpatch.pl --strict` and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run,
but the code here looks like it needs a fix. Feel free to get in touch
if the warning is not clear.
Sorry, this patchset is not ready, unless by RFC you meant - do not
review, because it is not ready. Then it is fine. But then *clearly
express* this in cover letter, so we know what you expect from us (and I
would not waste my time to go through all this).
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 13/14] riscv: dts: Introduce power domain node with simple-bus compatible
2024-12-03 15:52 ` Krzysztof Kozlowski
@ 2024-12-04 10:34 ` Michal Wilczynski
0 siblings, 0 replies; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-04 10:34 UTC (permalink / raw)
To: Krzysztof Kozlowski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 12/3/24 16:52, Krzysztof Kozlowski wrote:
> On 03/12/2024 14:41, Michal Wilczynski wrote:
>> The DRM Imagination GPU requires a power-domain driver, but the driver
>> for "thead,th1520-aon" is not yet available. To ensure that the 'aon'
>> node and its child 'pd' node are properly recognized and probed by the
>> kernel, add "simple-bus" to the compatible property of the 'aon' node.
>>
>> This change allows the kernel to treat the 'aon' node as a simple bus,
>> enabling the child nodes to be probed and initialized independently. It
>> ensures that the power domain can be managed appropriately until the
>> specific AON driver is developed.
>>
>> This commit introduces some errors while running dtbs_check, as the aon
>> doesn't have the dt-bindings yet.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>> arch/riscv/boot/dts/thead/th1520.dtsi | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
>> index 39d39059160d..58f93ad3eb6e 100644
>> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
>> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
>> @@ -6,6 +6,7 @@
>>
>> #include <dt-bindings/interrupt-controller/irq.h>
>> #include <dt-bindings/clock/thead,th1520-clk.h>
>> +#include <dt-bindings/power/thead,th1520-power.h>
>>
>> / {
>> compatible = "thead,th1520";
>> @@ -229,6 +230,16 @@ stmmac_axi_config: stmmac-axi-config {
>> snps,blen = <0 0 64 32 0 0 0>;
>> };
>>
>> + aon {
>> + compatible = "thead,th1520-aon", "simple-bus";
>
> 1. No, that's not a bus.
I understand that using "simple-bus" for the 'aon' node was not
appropriate. Since the 'aon' node isn't needed for testing this
patchset, I will remove it and move the power-domain device tree node to
the SoC. Future changes to the 'aon' node will be handled in a separate
patchset.
> 2. Please run scripts/checkpatch.pl and fix reported warnings. Then
> please run `scripts/checkpatch.pl --strict` and (probably) fix more
> warnings. Some warnings can be ignored, especially from --strict run,
> but the code here looks like it needs a fix. Feel free to get in touch
> if the warning is not clear.
>
> Sorry, this patchset is not ready, unless by RFC you meant - do not
> review, because it is not ready. Then it is fine. But then *clearly
> express* this in cover letter, so we know what you expect from us (and I
> would not waste my time to go through all this).
My intention with this patchset was to gather feedback on the overall
direction of the changes. I understand that clearer communication in the
cover letter would have been beneficial. Moving forward, I will ensure
that the patch's readiness and expectations are explicitly stated.
Thanks a lot for your review.
Michał
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v1 14/14] riscv: dts: Add GPU node to TH1520 device tree
2024-12-03 13:41 ` [RFC PATCH v1 00/14] Enable drm/imagination BXM-4-64 Support for LicheePi 4A Michal Wilczynski
` (12 preceding siblings ...)
2024-12-03 13:41 ` [RFC PATCH v1 13/14] riscv: dts: Introduce power domain node with simple-bus compatible Michal Wilczynski
@ 2024-12-03 13:41 ` Michal Wilczynski
2024-12-03 15:53 ` Krzysztof Kozlowski
13 siblings, 1 reply; 38+ messages in thread
From: Michal Wilczynski @ 2024-12-03 13:41 UTC (permalink / raw)
To: mturquette, sboyd, robh, krzk+dt, conor+dt, drew, guoren, wefu,
jassisinghbrar, paul.walmsley, palmer, aou, frank.binns,
matt.coster, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm, Michal Wilczynski
Add a device tree node for the IMG BXM-4-64 GPU present in the T-HEAD
TH1520 SoC used by the Lichee Pi 4A board. This node enables support for
the GPU using the drm/imagination driver.
By adding this node, the kernel can recognize and initialize the GPU,
providing graphics acceleration capabilities on the Lichee Pi 4A and
other boards based on the TH1520 SoC.
This commit is following convention introduced here [1].
Link: https://lore.kernel.org/all/20241118-sets-bxs-4-64-patch-v1-v2-1-3fd45d9fb0cf@imgtec.com/ [1]
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 58f93ad3eb6e..5023c0c29168 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -500,6 +500,18 @@ clk: clock-controller@ffef010000 {
#clock-cells = <1>;
};
+ gpu: gpu@ffef400000 {
+ compatible = "img,img-bxm-4-64", "img,img-rogue";
+ reg = <0xff 0xef400000 0x0 0x100000>;
+ interrupt-parent = <&plic>;
+ interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&vosys_clk CLK_GPU_CORE>,
+ <&vosys_clk CLK_GPU_CFG_ACLK>;
+ clock-names = "core", "sys";
+ power-domains = <&pd TH1520_AON_GPU_PD>;
+ status = "okay";
+ };
+
vosys_clk: clock-controller {
compatible = "thead,th1520-clk-vo";
thead,vosys-regmap = <&vosys_reg>;
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [RFC PATCH v1 14/14] riscv: dts: Add GPU node to TH1520 device tree
2024-12-03 13:41 ` [RFC PATCH v1 14/14] riscv: dts: Add GPU node to TH1520 device tree Michal Wilczynski
@ 2024-12-03 15:53 ` Krzysztof Kozlowski
0 siblings, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-03 15:53 UTC (permalink / raw)
To: Michal Wilczynski, mturquette, sboyd, robh, krzk+dt, conor+dt,
drew, guoren, wefu, jassisinghbrar, paul.walmsley, palmer, aou,
frank.binns, matt.coster, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, ulf.hansson, jszhang, m.szyprowski
Cc: linux-clk, devicetree, linux-kernel, linux-riscv, dri-devel,
linux-pm
On 03/12/2024 14:41, Michal Wilczynski wrote:
> Add a device tree node for the IMG BXM-4-64 GPU present in the T-HEAD
> TH1520 SoC used by the Lichee Pi 4A board. This node enables support for
> the GPU using the drm/imagination driver.
>
> By adding this node, the kernel can recognize and initialize the GPU,
> providing graphics acceleration capabilities on the Lichee Pi 4A and
> other boards based on the TH1520 SoC.
>
> This commit is following convention introduced here [1].
>
> Link: https://lore.kernel.org/all/20241118-sets-bxs-4-64-patch-v1-v2-1-3fd45d9fb0cf@imgtec.com/ [1]
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
> arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 58f93ad3eb6e..5023c0c29168 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -500,6 +500,18 @@ clk: clock-controller@ffef010000 {
> #clock-cells = <1>;
> };
>
> + gpu: gpu@ffef400000 {
> + compatible = "img,img-bxm-4-64", "img,img-rogue";
> + reg = <0xff 0xef400000 0x0 0x100000>;
> + interrupt-parent = <&plic>;
> + interrupts = <102 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&vosys_clk CLK_GPU_CORE>,
> + <&vosys_clk CLK_GPU_CFG_ACLK>;
> + clock-names = "core", "sys";
> + power-domains = <&pd TH1520_AON_GPU_PD>;
> + status = "okay";
Open existing DTSI and look how it is done. There is no single line like
'status = okay', so please do not introduce some entirely different
coding style.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 38+ messages in thread