2023-07-04 06:54:23

by Xingyu Wu

Subject: [RESEND PATCH v6 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC

[Resending because it has a error about examples in syscon bingdings
and has to be fixed.]

This patch serises are to add PLL clocks driver and providers by writing
and reading syscon registers for the StarFive JH7110 RISC-V SoC. And add
documentation and nodes to describe StarFive System Controller(syscon)
Registers. This patch serises are based on Linux 6.4.

PLLs are high speed, low jitter frequency synthesizers in JH7110.
Each PLL clock works in integer mode or fraction mode by some dividers,
and the dividers are set in several syscon registers.
The formula for calculating frequency is:
Fvco = Fref * (NI + NF) / M / Q1

The first patch adds docunmentation to describe PLL clock bindings,
and the second patch adds documentation to decribe syscon registers.
The patch 3 modifies the SYSCRG dibindings and adds PLL clock inputs.
The patch 4 adds driver to support PLL clocks for JH7110.
The patch 5 modifies the system clock driver and can select the PLL clock
source from PLL clocks driver. And the patch 6 adds the
stg/sys/aon syscon nodes for JH7110 SoC. The last patch modifies the
syscrg node in JH7110 dts file.

Changes since v5:
- Rebased on Linux 6.4.
- Patch 1 fixed some grammatical mistake.
- Patch 2 added the selection about properties from different syscon
modules and madethe example completed.
- Patch 3 dropped the 'optional' PLL clocks.

v5: https://lore.kernel.org/all/[email protected]/

Changes since v4:
- Rebased on Linux 6.4-rc6.
- Patch 2 dropped the example node about sys-syscon.
- Patch 3 used PLL clocks as one of optional items in SYSCRG bindings.
- Patch 4 used the patch instead about PLL clocks driver from Emil.
- Patch 5 retained the fixed factor PLL clocks as the optional source
about PLL clocks in SYSCRG clock driver.
- Patch 6 added the child node clock-controller as the complete
sys-syscon node and patch 7 dropped this part.

v4: https://lore.kernel.org/all/[email protected]/

Changes since v3:
- Rebased on Linux 6.4-rc1.
- Dropped the 'power-controller' property and used 'power-domain-cells'
instead in syscon binding.
- Used the data by of_device_id to get the syscon registers'
configuration include offset, mask and shift.

v3: https://lore.kernel.org/all/[email protected]/

Changes since v2:
- Rebased on latest JH7110 basic clock drivers.
- Added the complete documentation to describe syscon register.
- Added syscon node in JH7110 dts file.
- Modified the clock rate selection to match the closest rate in
PLL driver when setting rate.

v2: https://lore.kernel.org/all/[email protected]/

Changes since v1:
- Changed PLL clock node to be child of syscon node in dts.
- Modifed the definitions and names of function in PLL clock driver.
- Added commit to update syscon and syscrg dt-bindings.

v1: https://lore.kernel.org/all/[email protected]/

William Qiu (2):
dt-bindings: soc: starfive: Add StarFive syscon module
riscv: dts: starfive: jh7110: Add syscon nodes

Xingyu Wu (5):
dt-bindings: clock: Add StarFive JH7110 PLL clock generator
dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs
clk: starfive: Add StarFive JH7110 PLL clock driver
clk: starfive: jh7110-sys: Add PLL clocks source from DTS
riscv: dts: starfive: jh7110: Add PLL clocks source in SYSCRG node

.../bindings/clock/starfive,jh7110-pll.yaml | 46 ++
.../clock/starfive,jh7110-syscrg.yaml | 18 +-
.../soc/starfive/starfive,jh7110-syscon.yaml | 93 ++++
MAINTAINERS | 13 +
arch/riscv/boot/dts/starfive/jh7110.dtsi | 30 +-
drivers/clk/starfive/Kconfig | 9 +
drivers/clk/starfive/Makefile | 1 +
.../clk/starfive/clk-starfive-jh7110-pll.c | 507 ++++++++++++++++++
.../clk/starfive/clk-starfive-jh7110-sys.c | 45 +-
.../dt-bindings/clock/starfive,jh7110-crg.h | 6 +
10 files changed, 746 insertions(+), 22 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
create mode 100644 drivers/clk/starfive/clk-starfive-jh7110-pll.c

--
2.25.1

2023-07-04 07:00:12

by Xingyu Wu

Subject: [RESEND PATCH v6 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs

Add PLL clock inputs from PLL clock generator.

Signed-off-by: Xingyu Wu <[email protected]>
---
.../bindings/clock/starfive,jh7110-syscrg.yaml | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
index 84373ae31644..5ba0a885aa80 100644
--- a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
+++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
@@ -27,6 +27,9 @@ properties:
- description: External I2S RX left/right channel clock
- description: External TDM clock
- description: External audio master clock
+ - description: PLL0
+ - description: PLL1
+ - description: PLL2

- items:
- description: Main Oscillator (24 MHz)
@@ -38,6 +41,9 @@ properties:
- description: External I2S RX left/right channel clock
- description: External TDM clock
- description: External audio master clock
+ - description: PLL0
+ - description: PLL1
+ - description: PLL2

clock-names:
oneOf:
@@ -52,6 +58,9 @@ properties:
- const: i2srx_lrck_ext
- const: tdm_ext
- const: mclk_ext
+ - const: pll0_out
+ - const: pll1_out
+ - const: pll2_out

- items:
- const: osc
@@ -63,6 +72,9 @@ properties:
- const: i2srx_lrck_ext
- const: tdm_ext
- const: mclk_ext
+ - const: pll0_out
+ - const: pll1_out
+ - const: pll2_out

'#clock-cells':
const: 1
@@ -93,12 +105,14 @@ examples:
<&gmac1_rgmii_rxin>,
<&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
<&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
- <&tdm_ext>, <&mclk_ext>;
+ <&tdm_ext>, <&mclk_ext>,
+ <&pllclk 0>, <&pllclk 1>, <&pllclk 2>;
clock-names = "osc", "gmac1_rmii_refin",
"gmac1_rgmii_rxin",
"i2stx_bclk_ext", "i2stx_lrck_ext",
"i2srx_bclk_ext", "i2srx_lrck_ext",
- "tdm_ext", "mclk_ext";
+ "tdm_ext", "mclk_ext",
+ "pll0_out", "pll1_out", "pll2_out";
#clock-cells = <1>;
#reset-cells = <1>;
};
--
2.25.1

2023-07-04 07:00:11

by Xingyu Wu

Subject: [RESEND PATCH v6 4/7] clk: starfive: Add StarFive JH7110 PLL clock driver

Add driver for the StarFive JH7110 PLL clock controller
and they work by reading and setting syscon registers.

Co-developed-by: Emil Renner Berthing <[email protected]>
Signed-off-by: Emil Renner Berthing <[email protected]>
Signed-off-by: Xingyu Wu <[email protected]>
---
MAINTAINERS | 6 +
drivers/clk/starfive/Kconfig | 8 +
drivers/clk/starfive/Makefile | 1 +
.../clk/starfive/clk-starfive-jh7110-pll.c | 507 ++++++++++++++++++
4 files changed, 522 insertions(+)
create mode 100644 drivers/clk/starfive/clk-starfive-jh7110-pll.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 58ba04bd0bc8..b59b4d217991 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20127,6 +20127,12 @@ S: Supported
F: Documentation/devicetree/bindings/mmc/starfive*
F: drivers/mmc/host/dw_mmc-starfive.c

+STARFIVE JH7110 PLL CLOCK DRIVER
+M: Xingyu Wu <[email protected]>
+S: Supported
+F: Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
+F: drivers/clk/starfive/clk-starfive-jh7110-pll.*
+
STARFIVE JH7110 SYSCON
M: William Qiu <[email protected]>
M: Xingyu Wu <[email protected]>
diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
index 5d2333106f13..5195f7be5213 100644
--- a/drivers/clk/starfive/Kconfig
+++ b/drivers/clk/starfive/Kconfig
@@ -21,6 +21,14 @@ config CLK_STARFIVE_JH7100_AUDIO
Say Y or M here to support the audio clocks on the StarFive JH7100
SoC.

+config CLK_STARFIVE_JH7110_PLL
+ bool "StarFive JH7110 PLL clock support"
+ depends on ARCH_STARFIVE || COMPILE_TEST
+ default ARCH_STARFIVE
+ help
+ Say yes here to support the PLL clock controller on the
+ StarFive JH7110 SoC.
+
config CLK_STARFIVE_JH7110_SYS
bool "StarFive JH7110 system clock support"
depends on ARCH_STARFIVE || COMPILE_TEST
diff --git a/drivers/clk/starfive/Makefile b/drivers/clk/starfive/Makefile
index f3df7d957b1e..b48e539e52b0 100644
--- a/drivers/clk/starfive/Makefile
+++ b/drivers/clk/starfive/Makefile
@@ -4,5 +4,6 @@ obj-$(CONFIG_CLK_STARFIVE_JH71X0) += clk-starfive-jh71x0.o
obj-$(CONFIG_CLK_STARFIVE_JH7100) += clk-starfive-jh7100.o
obj-$(CONFIG_CLK_STARFIVE_JH7100_AUDIO) += clk-starfive-jh7100-audio.o

+obj-$(CONFIG_CLK_STARFIVE_JH7110_PLL) += clk-starfive-jh7110-pll.o
obj-$(CONFIG_CLK_STARFIVE_JH7110_SYS) += clk-starfive-jh7110-sys.o
obj-$(CONFIG_CLK_STARFIVE_JH7110_AON) += clk-starfive-jh7110-aon.o
diff --git a/drivers/clk/starfive/clk-starfive-jh7110-pll.c b/drivers/clk/starfive/clk-starfive-jh7110-pll.c
new file mode 100644
index 000000000000..b10c142d456d
--- /dev/null
+++ b/drivers/clk/starfive/clk-starfive-jh7110-pll.c
@@ -0,0 +1,507 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * StarFive JH7110 PLL Clock Generator Driver
+ *
+ * Copyright (C) 2023 StarFive Technology Co., Ltd.
+ * Copyright (C) 2023 Emil Renner Berthing <[email protected]>
+ *
+ * This driver is about to register JH7110 PLL clock generator and support ops.
+ * The JH7110 have three PLL clock, PLL0, PLL1 and PLL2.
+ * Each PLL clocks work in integer mode or fraction mode by some dividers,
+ * and the configuration registers and dividers are set in several syscon registers.
+ * The formula for calculating frequency is:
+ * Fvco = Fref * (NI + NF) / M / Q1
+ * Fref: OSC source clock rate
+ * NI: integer frequency dividing ratio of feedback divider, set by fbdiv[11:0].
+ * NF: fractional frequency dividing ratio, set by frac[23:0]. NF = frac[23:0] / 2^24 = 0 ~ 0.999.
+ * M: frequency dividing ratio of pre-divider, set by prediv[5:0].
+ * Q1: frequency dividing ratio of post divider, set by 2^postdiv1[1:0], eg. 1, 2, 4 or 8.
+ */
+
+#include <linux/bits.h>
+#include <linux/clk-provider.h>
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/starfive,jh7110-crg.h>
+
+/* this driver expects a 24MHz input frequency from the oscillator */
+#define JH7110_PLL_OSC_RATE 24000000UL
+
+#define JH7110_PLL0_PD_OFFSET 0x18
+#define JH7110_PLL0_DACPD_SHIFT 24
+#define JH7110_PLL0_DACPD_MASK BIT(24)
+#define JH7110_PLL0_DSMPD_SHIFT 25
+#define JH7110_PLL0_DSMPD_MASK BIT(25)
+#define JH7110_PLL0_FBDIV_OFFSET 0x1c
+#define JH7110_PLL0_FBDIV_SHIFT 0
+#define JH7110_PLL0_FBDIV_MASK GENMASK(11, 0)
+#define JH7110_PLL0_FRAC_OFFSET 0x20
+#define JH7110_PLL0_PREDIV_OFFSET 0x24
+
+#define JH7110_PLL1_PD_OFFSET 0x24
+#define JH7110_PLL1_DACPD_SHIFT 15
+#define JH7110_PLL1_DACPD_MASK BIT(15)
+#define JH7110_PLL1_DSMPD_SHIFT 16
+#define JH7110_PLL1_DSMPD_MASK BIT(16)
+#define JH7110_PLL1_FBDIV_OFFSET 0x24
+#define JH7110_PLL1_FBDIV_SHIFT 17
+#define JH7110_PLL1_FBDIV_MASK GENMASK(28, 17)
+#define JH7110_PLL1_FRAC_OFFSET 0x28
+#define JH7110_PLL1_PREDIV_OFFSET 0x2c
+
+#define JH7110_PLL2_PD_OFFSET 0x2c
+#define JH7110_PLL2_DACPD_SHIFT 15
+#define JH7110_PLL2_DACPD_MASK BIT(15)
+#define JH7110_PLL2_DSMPD_SHIFT 16
+#define JH7110_PLL2_DSMPD_MASK BIT(16)
+#define JH7110_PLL2_FBDIV_OFFSET 0x2c
+#define JH7110_PLL2_FBDIV_SHIFT 17
+#define JH7110_PLL2_FBDIV_MASK GENMASK(28, 17)
+#define JH7110_PLL2_FRAC_OFFSET 0x30
+#define JH7110_PLL2_PREDIV_OFFSET 0x34
+
+#define JH7110_PLL_FRAC_SHIFT 0
+#define JH7110_PLL_FRAC_MASK GENMASK(23, 0)
+#define JH7110_PLL_POSTDIV1_SHIFT 28
+#define JH7110_PLL_POSTDIV1_MASK GENMASK(29, 28)
+#define JH7110_PLL_PREDIV_SHIFT 0
+#define JH7110_PLL_PREDIV_MASK GENMASK(5, 0)
+
+enum jh7110_pll_mode {
+ JH7110_PLL_MODE_FRACTION,
+ JH7110_PLL_MODE_INTEGER,
+};
+
+struct jh7110_pll_preset {
+ unsigned long freq;
+ u32 frac; /* frac value should be decimals multiplied by 2^24 */
+ unsigned fbdiv : 12; /* fbdiv value should be 8 to 4095 */
+ unsigned prediv : 6;
+ unsigned postdiv1 : 2;
+ unsigned mode : 1;
+};
+
+struct jh7110_pll_info {
+ char *name;
+ const struct jh7110_pll_preset *presets;
+ unsigned int npresets;
+ struct {
+ unsigned int pd;
+ unsigned int fbdiv;
+ unsigned int frac;
+ unsigned int prediv;
+ } offsets;
+ struct {
+ u32 dacpd;
+ u32 dsmpd;
+ u32 fbdiv;
+ } masks;
+ struct {
+ char dacpd;
+ char dsmpd;
+ char fbdiv;
+ } shifts;
+};
+
+#define _JH7110_PLL(_idx, _name, _presets) \
+ [_idx] = { \
+ .name = _name, \
+ .presets = _presets, \
+ .npresets = ARRAY_SIZE(_presets), \
+ .offsets = { \
+ .pd = JH7110_PLL##_idx##_PD_OFFSET, \
+ .fbdiv = JH7110_PLL##_idx##_FBDIV_OFFSET, \
+ .frac = JH7110_PLL##_idx##_FRAC_OFFSET, \
+ .prediv = JH7110_PLL##_idx##_PREDIV_OFFSET, \
+ }, \
+ .masks = { \
+ .dacpd = JH7110_PLL##_idx##_DACPD_MASK, \
+ .dsmpd = JH7110_PLL##_idx##_DSMPD_MASK, \
+ .fbdiv = JH7110_PLL##_idx##_FBDIV_MASK, \
+ }, \
+ .shifts = { \
+ .dacpd = JH7110_PLL##_idx##_DACPD_SHIFT, \
+ .dsmpd = JH7110_PLL##_idx##_DSMPD_SHIFT, \
+ .fbdiv = JH7110_PLL##_idx##_FBDIV_SHIFT, \
+ }, \
+ }
+#define JH7110_PLL(idx, name, presets) _JH7110_PLL(idx, name, presets)
+
+struct jh7110_pll_data {
+ struct clk_hw hw;
+ unsigned int idx;
+};
+
+struct jh7110_pll_priv {
+ struct device *dev;
+ struct regmap *regmap;
+ struct jh7110_pll_data pll[JH7110_PLLCLK_END];
+};
+
+struct jh7110_pll_regvals {
+ u32 dacpd;
+ u32 dsmpd;
+ u32 fbdiv;
+ u32 frac;
+ u32 postdiv1;
+ u32 prediv;
+};
+
+/*
+ * Because the pll frequency is relatively fixed,
+ * it cannot be set arbitrarily, so it needs a specific configuration.
+ * PLL0 frequency should be multiple of 125MHz (USB frequency).
+ */
+static const struct jh7110_pll_preset jh7110_pll0_presets[] = {
+ {
+ .freq = 375000000,
+ .fbdiv = 125,
+ .prediv = 8,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 500000000,
+ .fbdiv = 125,
+ .prediv = 6,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 625000000,
+ .fbdiv = 625,
+ .prediv = 24,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 750000000,
+ .fbdiv = 125,
+ .prediv = 4,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 875000000,
+ .fbdiv = 875,
+ .prediv = 24,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 1000000000,
+ .fbdiv = 125,
+ .prediv = 3,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 1250000000,
+ .fbdiv = 625,
+ .prediv = 12,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 1375000000,
+ .fbdiv = 1375,
+ .prediv = 24,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 1500000000,
+ .fbdiv = 125,
+ .prediv = 2,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ },
+};
+
+static const struct jh7110_pll_preset jh7110_pll1_presets[] = {
+ {
+ .freq = 1066000000,
+ .fbdiv = 533,
+ .prediv = 12,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 1200000000,
+ .fbdiv = 50,
+ .prediv = 1,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 1400000000,
+ .fbdiv = 350,
+ .prediv = 6,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 1600000000,
+ .fbdiv = 200,
+ .prediv = 3,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ },
+};
+
+static const struct jh7110_pll_preset jh7110_pll2_presets[] = {
+ {
+ .freq = 1188000000,
+ .fbdiv = 99,
+ .prediv = 2,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ }, {
+ .freq = 1228800000,
+ .fbdiv = 256,
+ .prediv = 5,
+ .postdiv1 = 0,
+ .mode = JH7110_PLL_MODE_INTEGER,
+ },
+};
+
+static const struct jh7110_pll_info jh7110_plls[JH7110_PLLCLK_END] = {
+ JH7110_PLL(JH7110_CLK_PLL0_OUT, "pll0_out", jh7110_pll0_presets),
+ JH7110_PLL(JH7110_CLK_PLL1_OUT, "pll1_out", jh7110_pll1_presets),
+ JH7110_PLL(JH7110_CLK_PLL2_OUT, "pll2_out", jh7110_pll2_presets),
+};
+
+static struct jh7110_pll_data *jh7110_pll_data_from(struct clk_hw *hw)
+{
+ return container_of(hw, struct jh7110_pll_data, hw);
+}
+
+static struct jh7110_pll_priv *jh7110_pll_priv_from(struct jh7110_pll_data *pll)
+{
+ return container_of(pll, struct jh7110_pll_priv, pll[pll->idx]);
+}
+
+static void jh7110_pll_regvals_get(struct regmap *regmap,
+ const struct jh7110_pll_info *info,
+ struct jh7110_pll_regvals *ret)
+{
+ u32 val;
+
+ regmap_read(regmap, info->offsets.pd, &val);
+ ret->dacpd = (val & info->masks.dacpd) >> info->shifts.dacpd;
+ ret->dsmpd = (val & info->masks.dsmpd) >> info->shifts.dsmpd;
+
+ regmap_read(regmap, info->offsets.fbdiv, &val);
+ ret->fbdiv = (val & info->masks.fbdiv) >> info->shifts.fbdiv;
+
+ regmap_read(regmap, info->offsets.frac, &val);
+ ret->frac = (val & JH7110_PLL_FRAC_MASK) >> JH7110_PLL_FRAC_SHIFT;
+ ret->postdiv1 = (val & JH7110_PLL_POSTDIV1_MASK) >> JH7110_PLL_POSTDIV1_SHIFT;
+
+ regmap_read(regmap, info->offsets.prediv, &val);
+ ret->prediv = (val & JH7110_PLL_PREDIV_MASK) >> JH7110_PLL_PREDIV_SHIFT;
+}
+
+static unsigned long jh7110_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+ struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
+ struct jh7110_pll_priv *priv = jh7110_pll_priv_from(pll);
+ struct jh7110_pll_regvals val;
+ unsigned long rate;
+
+ jh7110_pll_regvals_get(priv->regmap, &jh7110_plls[pll->idx], &val);
+
+ /*
+ * dacpd = dsmpd = 0: fraction mode
+ * dacpd = dsmpd = 1: integer mode, frac value ignored
+ *
+ * rate = parent * (fbdiv + frac/2^24) / prediv / 2^postdiv1
+ * = (parent * fbdiv + parent * frac / 2^24) / (prediv * 2^postdiv1)
+ */
+ if (val.dacpd == 0 && val.dsmpd == 0)
+ rate = parent_rate * val.frac / (1UL << 24);
+ else if (val.dacpd == 1 && val.dsmpd == 1)
+ rate = 0;
+ else
+ return 0;
+
+ rate += parent_rate * val.fbdiv;
+ rate /= val.prediv << val.postdiv1;
+
+ return rate;
+}
+
+static int jh7110_pll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
+{
+ struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
+ const struct jh7110_pll_info *info = &jh7110_plls[pll->idx];
+ const struct jh7110_pll_preset *selected = &info->presets[0];
+ unsigned int idx;
+
+ /* if the parent rate doesn't match our expectations the presets won't work */
+ if (req->best_parent_rate != JH7110_PLL_OSC_RATE) {
+ req->rate = jh7110_pll_recalc_rate(hw, req->best_parent_rate);
+ return 0;
+ }
+
+ /* find highest rate lower or equal to the requested rate */
+ for (idx = 1; idx < info->npresets; idx++) {
+ const struct jh7110_pll_preset *val = &info->presets[idx];
+
+ if (req->rate < val->freq)
+ break;
+
+ selected = val;
+ }
+
+ req->rate = selected->freq;
+ return 0;
+}
+
+static int jh7110_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
+ struct jh7110_pll_priv *priv = jh7110_pll_priv_from(pll);
+ const struct jh7110_pll_info *info = &jh7110_plls[pll->idx];
+ const struct jh7110_pll_preset *val;
+ unsigned int idx;
+
+ /* if the parent rate doesn't match our expectations the presets won't work */
+ if (parent_rate != JH7110_PLL_OSC_RATE)
+ return -EINVAL;
+
+ for (idx = 0, val = &info->presets[0]; idx < info->npresets; idx++, val++) {
+ if (val->freq == rate)
+ goto found;
+ }
+ return -EINVAL;
+
+found:
+ if (val->mode == JH7110_PLL_MODE_FRACTION)
+ regmap_update_bits(priv->regmap, info->offsets.frac, JH7110_PLL_FRAC_MASK,
+ val->frac << JH7110_PLL_FRAC_SHIFT);
+
+ regmap_update_bits(priv->regmap, info->offsets.pd, info->masks.dacpd,
+ (u32)val->mode << info->shifts.dacpd);
+ regmap_update_bits(priv->regmap, info->offsets.pd, info->masks.dsmpd,
+ (u32)val->mode << info->shifts.dsmpd);
+ regmap_update_bits(priv->regmap, info->offsets.prediv, JH7110_PLL_PREDIV_MASK,
+ (u32)val->prediv << JH7110_PLL_PREDIV_SHIFT);
+ regmap_update_bits(priv->regmap, info->offsets.fbdiv, info->masks.fbdiv,
+ val->fbdiv << info->shifts.fbdiv);
+ regmap_update_bits(priv->regmap, info->offsets.frac, JH7110_PLL_POSTDIV1_MASK,
+ (u32)val->postdiv1 << JH7110_PLL_POSTDIV1_SHIFT);
+
+ return 0;
+}
+
+#ifdef CONFIG_DEBUG_FS
+static int jh7110_pll_registers_read(struct seq_file *s, void *unused)
+{
+ struct jh7110_pll_data *pll = s->private;
+ struct jh7110_pll_priv *priv = jh7110_pll_priv_from(pll);
+ struct jh7110_pll_regvals val;
+
+ jh7110_pll_regvals_get(priv->regmap, &jh7110_plls[pll->idx], &val);
+
+ seq_printf(s, "fbdiv=%u\n"
+ "frac=%u\n"
+ "prediv=%u\n"
+ "postdiv1=%u\n"
+ "dacpd=%u\n"
+ "dsmpd=%u\n",
+ val.fbdiv, val.frac, val.prediv, val.postdiv1,
+ val.dacpd, val.dsmpd);
+
+ return 0;
+}
+
+static int jh7110_pll_registers_open(struct inode *inode, struct file *f)
+{
+ return single_open(f, jh7110_pll_registers_read, inode->i_private);
+}
+
+static const struct file_operations jh7110_pll_registers_ops = {
+ .owner = THIS_MODULE,
+ .open = jh7110_pll_registers_open,
+ .release = single_release,
+ .read = seq_read,
+ .llseek = seq_lseek
+};
+
+static void jh7110_pll_debug_init(struct clk_hw *hw, struct dentry *dentry)
+{
+ struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
+
+ debugfs_create_file("registers", 0400, dentry, pll,
+ &jh7110_pll_registers_ops);
+}
+#else
+#define jh7110_pll_debug_init NULL
+#endif
+
+static const struct clk_ops jh7110_pll_ops = {
+ .recalc_rate = jh7110_pll_recalc_rate,
+ .determine_rate = jh7110_pll_determine_rate,
+ .set_rate = jh7110_pll_set_rate,
+ .debug_init = jh7110_pll_debug_init,
+};
+
+static struct clk_hw *jh7110_pll_get(struct of_phandle_args *clkspec, void *data)
+{
+ struct jh7110_pll_priv *priv = data;
+ unsigned int idx = clkspec->args[0];
+
+ if (idx < JH7110_PLLCLK_END)
+ return &priv->pll[idx].hw;
+
+ return ERR_PTR(-EINVAL);
+}
+
+static int jh7110_pll_probe(struct platform_device *pdev)
+{
+ struct jh7110_pll_priv *priv;
+ unsigned int idx;
+ int ret;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = &pdev->dev;
+ priv->regmap = syscon_node_to_regmap(priv->dev->of_node->parent);
+ if (IS_ERR(priv->regmap))
+ return PTR_ERR(priv->regmap);
+
+ for (idx = 0; idx < JH7110_PLLCLK_END; idx++) {
+ struct clk_parent_data parents = {
+ .index = 0,
+ };
+ struct clk_init_data init = {
+ .name = jh7110_plls[idx].name,
+ .ops = &jh7110_pll_ops,
+ .parent_data = &parents,
+ .num_parents = 1,
+ .flags = 0,
+ };
+ struct jh7110_pll_data *pll = &priv->pll[idx];
+
+ pll->hw.init = &init;
+ pll->idx = idx;
+
+ ret = devm_clk_hw_register(&pdev->dev, &pll->hw);
+ if (ret)
+ return ret;
+ }
+
+ return devm_of_clk_add_hw_provider(&pdev->dev, jh7110_pll_get, priv);
+}
+
+static const struct of_device_id jh7110_pll_match[] = {
+ { .compatible = "starfive,jh7110-pll" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, jh7110_pll_match);
+
+static struct platform_driver jh7110_pll_driver = {
+ .driver = {
+ .name = "clk-starfive-jh7110-pll",
+ .of_match_table = jh7110_pll_match,
+ },
+};
+builtin_platform_driver_probe(jh7110_pll_driver, jh7110_pll_probe);
--
2.25.1

2023-07-04 07:01:59

by Xingyu Wu

Subject: [RESEND PATCH v6 1/7] dt-bindings: clock: Add StarFive JH7110 PLL clock generator

Add bindings for the PLL clock generator on the JH7110 RISC-V SoC.

Reviewed-by: Conor Dooley <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Xingyu Wu <[email protected]>
---
.../bindings/clock/starfive,jh7110-pll.yaml | 46 +++++++++++++++++++
.../dt-bindings/clock/starfive,jh7110-crg.h | 6 +++
2 files changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml

diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
new file mode 100644
index 000000000000..beb78add5a8d
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/starfive,jh7110-pll.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 PLL Clock Generator
+
+description:
+ These PLLs are high speed, low jitter frequency synthesizers in JH7110.
+ Each PLL works in integer mode or fraction mode, with configuration
+ registers in the sys syscon. So the PLLs node should be a child of
+ SYS-SYSCON node.
+ The formula for calculating frequency is
+ Fvco = Fref * (NI + NF) / M / Q1
+
+maintainers:
+ - Xingyu Wu <[email protected]>
+
+properties:
+ compatible:
+ const: starfive,jh7110-pll
+
+ clocks:
+ maxItems: 1
+ description: Main Oscillator (24 MHz)
+
+ '#clock-cells':
+ const: 1
+ description:
+ See <dt-bindings/clock/starfive,jh7110-crg.h> for valid indices.
+
+required:
+ - compatible
+ - clocks
+ - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ clock-controller {
+ compatible = "starfive,jh7110-pll";
+ clocks = <&osc>;
+ #clock-cells = <1>;
+ };
diff --git a/include/dt-bindings/clock/starfive,jh7110-crg.h b/include/dt-bindings/clock/starfive,jh7110-crg.h
index 06257bfd9ac1..086a6ddcf380 100644
--- a/include/dt-bindings/clock/starfive,jh7110-crg.h
+++ b/include/dt-bindings/clock/starfive,jh7110-crg.h
@@ -6,6 +6,12 @@
#ifndef __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
#define __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__

+/* PLL clocks */
+#define JH7110_CLK_PLL0_OUT 0
+#define JH7110_CLK_PLL1_OUT 1
+#define JH7110_CLK_PLL2_OUT 2
+#define JH7110_PLLCLK_END 3
+
/* SYSCRG clocks */
#define JH7110_SYSCLK_CPU_ROOT 0
#define JH7110_SYSCLK_CPU_CORE 1
--
2.25.1

2023-07-04 07:02:44

by Xingyu Wu

Subject: [RESEND PATCH v6 7/7] riscv: dts: starfive: jh7110: Add PLL clocks source in SYSCRG node

Add PLL clocks input from PLL clocks driver in SYSCRG node.

Reviewed-by: Conor Dooley <[email protected]>
Signed-off-by: Xingyu Wu <[email protected]>
---
arch/riscv/boot/dts/starfive/jh7110.dtsi | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 11dd4c9d64b0..cdfd036a0e6c 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -452,12 +452,16 @@ syscrg: clock-controller@13020000 {
<&gmac1_rgmii_rxin>,
<&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
<&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
- <&tdm_ext>, <&mclk_ext>;
+ <&tdm_ext>, <&mclk_ext>,
+ <&pllclk JH7110_CLK_PLL0_OUT>,
+ <&pllclk JH7110_CLK_PLL1_OUT>,
+ <&pllclk JH7110_CLK_PLL2_OUT>;
clock-names = "osc", "gmac1_rmii_refin",
"gmac1_rgmii_rxin",
"i2stx_bclk_ext", "i2stx_lrck_ext",
"i2srx_bclk_ext", "i2srx_lrck_ext",
- "tdm_ext", "mclk_ext";
+ "tdm_ext", "mclk_ext",
+ "pll0_out", "pll1_out", "pll2_out";
#clock-cells = <1>;
#reset-cells = <1>;
};
--
2.25.1

2023-07-04 07:09:17

by Xingyu Wu

Subject: [RESEND PATCH v6 6/7] riscv: dts: starfive: jh7110: Add syscon nodes

From: William Qiu <[email protected]>

Add stg_syscon/sys_syscon/aon_syscon/PLL nodes for JH7110 SoC.

Reviewed-by: Conor Dooley <[email protected]>
Co-developed-by: Xingyu Wu <[email protected]>
Signed-off-by: Xingyu Wu <[email protected]>
Signed-off-by: William Qiu <[email protected]>
---
arch/riscv/boot/dts/starfive/jh7110.dtsi | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 4c5fdb905da8..11dd4c9d64b0 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -353,6 +353,11 @@ i2c2: i2c@10050000 {
status = "disabled";
};

+ stg_syscon: syscon@10240000 {
+ compatible = "starfive,jh7110-stg-syscon", "syscon";
+ reg = <0x0 0x10240000 0x0 0x1000>;
+ };
+
uart3: serial@12000000 {
compatible = "snps,dw-apb-uart";
reg = <0x0 0x12000000 0x0 0x10000>;
@@ -457,6 +462,17 @@ syscrg: clock-controller@13020000 {
#reset-cells = <1>;
};

+ sys_syscon: syscon@13030000 {
+ compatible = "starfive,jh7110-sys-syscon", "syscon", "simple-mfd";
+ reg = <0x0 0x13030000 0x0 0x1000>;
+
+ pllclk: clock-controller {
+ compatible = "starfive,jh7110-pll";
+ clocks = <&osc>;
+ #clock-cells = <1>;
+ };
+ };
+
sysgpio: pinctrl@13040000 {
compatible = "starfive,jh7110-sys-pinctrl";
reg = <0x0 0x13040000 0x0 0x10000>;
@@ -486,6 +502,12 @@ aoncrg: clock-controller@17000000 {
#reset-cells = <1>;
};

+ aon_syscon: syscon@17010000 {
+ compatible = "starfive,jh7110-aon-syscon", "syscon";
+ reg = <0x0 0x17010000 0x0 0x1000>;
+ #power-domain-cells = <1>;
+ };
+
aongpio: pinctrl@17020000 {
compatible = "starfive,jh7110-aon-pinctrl";
reg = <0x0 0x17020000 0x0 0x10000>;
--
2.25.1

2023-07-04 07:09:18

by Xingyu Wu

Subject: [RESEND PATCH v6 2/7] dt-bindings: soc: starfive: Add StarFive syscon module

From: William Qiu <[email protected]>

Add documentation to describe StarFive System Controller Registers.

Co-developed-by: Xingyu Wu <[email protected]>
Signed-off-by: Xingyu Wu <[email protected]>
Signed-off-by: William Qiu <[email protected]>
---
.../soc/starfive/starfive,jh7110-syscon.yaml | 93 +++++++++++++++++++
MAINTAINERS | 7 ++
2 files changed, 100 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml

diff --git a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
new file mode 100644
index 000000000000..0039319e91fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/starfive/starfive,jh7110-syscon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 SoC system controller
+
+maintainers:
+ - William Qiu <[email protected]>
+
+description:
+ The StarFive JH7110 SoC system controller provides register information such
+ as offset, mask and shift to configure related modules such as MMC and PCIe.
+
+properties:
+ compatible:
+ oneOf:
+ - items:
+ - const: starfive,jh7110-sys-syscon
+ - const: syscon
+ - const: simple-mfd
+ - items:
+ - enum:
+ - starfive,jh7110-aon-syscon
+ - starfive,jh7110-stg-syscon
+ - const: syscon
+
+ reg:
+ maxItems: 1
+
+ clock-controller:
+ $ref: /schemas/clock/starfive,jh7110-pll.yaml#
+ type: object
+
+ "#power-domain-cells":
+ const: 1
+
+required:
+ - compatible
+ - reg
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: starfive,jh7110-sys-syscon
+ then:
+ required:
+ - clock-controller
+ else:
+ properties:
+ clock-controller: false
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: starfive,jh7110-aon-syscon
+ then:
+ required:
+ - "#power-domain-cells"
+ else:
+ properties:
+ "#power-domain-cells": false
+
+additionalProperties: false
+
+examples:
+ - |
+ syscon@10240000 {
+ compatible = "starfive,jh7110-stg-syscon", "syscon";
+ reg = <0x10240000 0x1000>;
+ };
+
+ syscon@13030000 {
+ compatible = "starfive,jh7110-sys-syscon", "syscon", "simple-mfd";
+ reg = <0x13030000 0x1000>;
+
+ clock-controller {
+ compatible = "starfive,jh7110-pll";
+ clocks = <&osc>;
+ #clock-cells = <1>;
+ };
+ };
+
+ syscon@17010000 {
+ compatible = "starfive,jh7110-aon-syscon", "syscon";
+ reg = <0x17010000 0x1000>;
+ #power-domain-cells = <1>;
+ };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 35e19594640d..58ba04bd0bc8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20127,6 +20127,12 @@ S: Supported
F: Documentation/devicetree/bindings/mmc/starfive*
F: drivers/mmc/host/dw_mmc-starfive.c

+STARFIVE JH7110 SYSCON
+M: William Qiu <[email protected]>
+M: Xingyu Wu <[email protected]>
+S: Supported
+F: Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
+
STARFIVE JH71X0 CLOCK DRIVERS
M: Emil Renner Berthing <[email protected]>
M: Hal Feng <[email protected]>
@@ -20164,6 +20170,7 @@ STARFIVE SOC DRIVERS
M: Conor Dooley <[email protected]>
S: Maintained
T: git https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/
+F: Documentation/devicetree/bindings/soc/starfive/
F: drivers/soc/starfive/

STARFIVE TRNG DRIVER
--
2.25.1

2023-07-04 07:09:45

by Xingyu Wu

Subject: [RESEND PATCH v6 5/7] clk: starfive: jh7110-sys: Add PLL clocks source from DTS

Modify PLL clocks source to be got from DTS or
the fixed factor clocks.

Signed-off-by: Xingyu Wu <[email protected]>
---
drivers/clk/starfive/Kconfig | 1 +
.../clk/starfive/clk-starfive-jh7110-sys.c | 45 +++++++++++--------
2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
index 5195f7be5213..978b78ec08b1 100644
--- a/drivers/clk/starfive/Kconfig
+++ b/drivers/clk/starfive/Kconfig
@@ -35,6 +35,7 @@ config CLK_STARFIVE_JH7110_SYS
select AUXILIARY_BUS
select CLK_STARFIVE_JH71X0
select RESET_STARFIVE_JH7110 if RESET_CONTROLLER
+ select CLK_STARFIVE_JH7110_PLL
default ARCH_STARFIVE
help
Say yes here to support the system clock controller on the
diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
index e6031345ef05..d56f48013388 100644
--- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
+++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
@@ -7,6 +7,7 @@
*/

#include <linux/auxiliary_bus.h>
+#include <linux/clk.h>
#include <linux/clk-provider.h>
#include <linux/init.h>
#include <linux/io.h>
@@ -386,6 +387,7 @@ EXPORT_SYMBOL_GPL(jh7110_reset_controller_register);

static int __init jh7110_syscrg_probe(struct platform_device *pdev)
{
+ bool use_fixed_pll = true; /* PLL clocks use fixed factor clocks or PLL driver */
struct jh71x0_clk_priv *priv;
unsigned int idx;
int ret;
@@ -402,28 +404,29 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
if (IS_ERR(priv->base))
return PTR_ERR(priv->base);

- /*
- * These PLL clocks are not actually fixed factor clocks and can be
- * controlled by the syscon registers of JH7110. They will be dropped
- * and registered in the PLL clock driver instead.
- */
+ if (!IS_ERR(devm_clk_get(priv->dev, "pll0_out")))
+ use_fixed_pll = false; /* can get pll clocks from PLL driver */
+
+ /* Use fixed factor clocks if can not get the PLL clocks from DTS */
+ if (use_fixed_pll) {
/* 24MHz -> 1000.0MHz */
- priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
- "osc", 0, 125, 3);
- if (IS_ERR(priv->pll[0]))
- return PTR_ERR(priv->pll[0]);
+ priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
+ "osc", 0, 125, 3);
+ if (IS_ERR(priv->pll[0]))
+ return PTR_ERR(priv->pll[0]);

/* 24MHz -> 1066.0MHz */
- priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
- "osc", 0, 533, 12);
- if (IS_ERR(priv->pll[1]))
- return PTR_ERR(priv->pll[1]);
+ priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
+ "osc", 0, 533, 12);
+ if (IS_ERR(priv->pll[1]))
+ return PTR_ERR(priv->pll[1]);

/* 24MHz -> 1188.0MHz */
- priv->pll[2] = devm_clk_hw_register_fixed_factor(priv->dev, "pll2_out",
- "osc", 0, 99, 2);
- if (IS_ERR(priv->pll[2]))
- return PTR_ERR(priv->pll[2]);
+ priv->pll[2] = devm_clk_hw_register_fixed_factor(priv->dev, "pll2_out",
+ "osc", 0, 99, 2);
+ if (IS_ERR(priv->pll[2]))
+ return PTR_ERR(priv->pll[2]);
+ }

for (idx = 0; idx < JH7110_SYSCLK_END; idx++) {
u32 max = jh7110_sysclk_data[idx].max;
@@ -462,8 +465,14 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
parents[i].fw_name = "tdm_ext";
else if (pidx == JH7110_SYSCLK_MCLK_EXT)
parents[i].fw_name = "mclk_ext";
- else
+ else if (use_fixed_pll)
parents[i].hw = priv->pll[pidx - JH7110_SYSCLK_PLL0_OUT];
+ else if (pidx == JH7110_SYSCLK_PLL0_OUT)
+ parents[i].fw_name = "pll0_out";
+ else if (pidx == JH7110_SYSCLK_PLL1_OUT)
+ parents[i].fw_name = "pll1_out";
+ else if (pidx == JH7110_SYSCLK_PLL2_OUT)
+ parents[i].fw_name = "pll2_out";
}

clk->hw.init = &init;
--
2.25.1

2023-07-04 23:16:28

by Conor Dooley

Subject: Re: [RESEND PATCH v6 5/7] clk: starfive: jh7110-sys: Add PLL clocks source from DTS

Hey Xingyu,

On Tue, Jul 04, 2023 at 02:46:08PM +0800, Xingyu Wu wrote:
> Modify PLL clocks source to be got from DTS or
> the fixed factor clocks.

Again, would be good to mention here that the reason we are keeping the
fixed-factor clocks is for backwards compatibility with the existing DT.

Otherwise, this seems good to me, thanks.

Attachments:

2023-07-04 23:27:04

by Conor Dooley

Subject: Re: [RESEND PATCH v6 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC

Emil,

On Tue, Jul 04, 2023 at 02:46:03PM +0800, Xingyu Wu wrote:
> [Resending because it has a error about examples in syscon bingdings
> and has to be fixed.]
>
> This patch serises are to add PLL clocks driver and providers by writing
> and reading syscon registers for the StarFive JH7110 RISC-V SoC. And add
> documentation and nodes to describe StarFive System Controller(syscon)
> Registers. This patch serises are based on Linux 6.4.

Could you take a look at this series when you get a chance, please?
Would be good to finally get it merged since the syscon bits are a dep
for a few other things :)

Thanks!

Conor.

> William Qiu (2):
> dt-bindings: soc: starfive: Add StarFive syscon module
> riscv: dts: starfive: jh7110: Add syscon nodes
>
> Xingyu Wu (5):
> dt-bindings: clock: Add StarFive JH7110 PLL clock generator
> dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs
> clk: starfive: Add StarFive JH7110 PLL clock driver
> clk: starfive: jh7110-sys: Add PLL clocks source from DTS
> riscv: dts: starfive: jh7110: Add PLL clocks source in SYSCRG node
>
> .../bindings/clock/starfive,jh7110-pll.yaml | 46 ++
> .../clock/starfive,jh7110-syscrg.yaml | 18 +-
> .../soc/starfive/starfive,jh7110-syscon.yaml | 93 ++++
> MAINTAINERS | 13 +
> arch/riscv/boot/dts/starfive/jh7110.dtsi | 30 +-
> drivers/clk/starfive/Kconfig | 9 +
> drivers/clk/starfive/Makefile | 1 +
> .../clk/starfive/clk-starfive-jh7110-pll.c | 507 ++++++++++++++++++
> .../clk/starfive/clk-starfive-jh7110-sys.c | 45 +-
> .../dt-bindings/clock/starfive,jh7110-crg.h | 6 +
> 10 files changed, 746 insertions(+), 22 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
> create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> create mode 100644 drivers/clk/starfive/clk-starfive-jh7110-pll.c
>
> --
> 2.25.1
>

Attachments:

2023-07-04 23:30:53

by Conor Dooley

Subject: Re: [RESEND PATCH v6 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs

On Tue, Jul 04, 2023 at 02:46:06PM +0800, Xingyu Wu wrote:
> Add PLL clock inputs from PLL clock generator.
>
> Signed-off-by: Xingyu Wu <[email protected]>

As expected this produces warnings for the existing, in-tree,
devicetrees which go away when the later dts patches are applied.
It'd be good to mention that its intentional if you end up sending
another version of the series.

Otherwise, this looks good to me too.

Reviewed-by: Conor Dooley <[email protected]>

Cheers,
Conor.

> ---
> .../bindings/clock/starfive,jh7110-syscrg.yaml | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> index 84373ae31644..5ba0a885aa80 100644
> --- a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> +++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> @@ -27,6 +27,9 @@ properties:
> - description: External I2S RX left/right channel clock
> - description: External TDM clock
> - description: External audio master clock
> + - description: PLL0
> + - description: PLL1
> + - description: PLL2
>
> - items:
> - description: Main Oscillator (24 MHz)
> @@ -38,6 +41,9 @@ properties:
> - description: External I2S RX left/right channel clock
> - description: External TDM clock
> - description: External audio master clock
> + - description: PLL0
> + - description: PLL1
> + - description: PLL2
>
> clock-names:
> oneOf:
> @@ -52,6 +58,9 @@ properties:
> - const: i2srx_lrck_ext
> - const: tdm_ext
> - const: mclk_ext
> + - const: pll0_out
> + - const: pll1_out
> + - const: pll2_out
>
> - items:
> - const: osc
> @@ -63,6 +72,9 @@ properties:
> - const: i2srx_lrck_ext
> - const: tdm_ext
> - const: mclk_ext
> + - const: pll0_out
> + - const: pll1_out
> + - const: pll2_out
>
> '#clock-cells':
> const: 1
> @@ -93,12 +105,14 @@ examples:
> <&gmac1_rgmii_rxin>,
> <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
> <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
> - <&tdm_ext>, <&mclk_ext>;
> + <&tdm_ext>, <&mclk_ext>,
> + <&pllclk 0>, <&pllclk 1>, <&pllclk 2>;
> clock-names = "osc", "gmac1_rmii_refin",
> "gmac1_rgmii_rxin",
> "i2stx_bclk_ext", "i2stx_lrck_ext",
> "i2srx_bclk_ext", "i2srx_lrck_ext",
> - "tdm_ext", "mclk_ext";
> + "tdm_ext", "mclk_ext",
> + "pll0_out", "pll1_out", "pll2_out";
> #clock-cells = <1>;
> #reset-cells = <1>;
> };
> --
> 2.25.1
>

Attachments:

2023-07-04 23:34:03

by Conor Dooley

Subject: Re: [RESEND PATCH v6 2/7] dt-bindings: soc: starfive: Add StarFive syscon module

On Tue, Jul 04, 2023 at 02:46:05PM +0800, Xingyu Wu wrote:
> From: William Qiu <[email protected]>
>
> Add documentation to describe StarFive System Controller Registers.
>
> Co-developed-by: Xingyu Wu <[email protected]>
> Signed-off-by: Xingyu Wu <[email protected]>
> Signed-off-by: William Qiu <[email protected]>

This looks good to me now, thanks for keeping at it.

Reviewed-by: Conor Dooley <[email protected]>

Cheers,
Conor.

Attachments:

2023-07-05 06:41:23

by Krzysztof Kozlowski

Subject: Re: [RESEND PATCH v6 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC

On 04/07/2023 08:46, Xingyu Wu wrote:
> [Resending because it has a error about examples in syscon bingdings
> and has to be fixed.]

Resending means you sent the same version. If you did not change
anything, then it will fail, right? If you changed something, it is new
version.

Best regards,
Krzysztof

2023-07-05 06:48:09

by Krzysztof Kozlowski

Subject: Re: [RESEND PATCH v6 2/7] dt-bindings: soc: starfive: Add StarFive syscon module

On 04/07/2023 08:46, Xingyu Wu wrote:
> From: William Qiu <[email protected]>
>
> Add documentation to describe StarFive System Controller Registers.
>
> Co-developed-by: Xingyu Wu <[email protected]>
> Signed-off-by: Xingyu Wu <[email protected]>
> Signed-off-by: William Qiu <[email protected]>

If you resent the same buggy code, then it is still no point to review.

Best regards,
Krzysztof

2023-07-07 08:08:55

by Xingyu Wu

Subject: Re: [RESEND PATCH v6 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs

On 2023/7/5 6:23, Conor Dooley wrote:
> On Tue, Jul 04, 2023 at 02:46:06PM +0800, Xingyu Wu wrote:
>> Add PLL clock inputs from PLL clock generator.
>>
>> Signed-off-by: Xingyu Wu <[email protected]>
>
> As expected this produces warnings for the existing, in-tree,
> devicetrees which go away when the later dts patches are applied.
> It'd be good to mention that its intentional if you end up sending
> another version of the series.
>
> Otherwise, this looks good to me too.
>
> Reviewed-by: Conor Dooley <[email protected]>
>

Thanks, I will add the mentions in next version.

Best regards,
Xingyu Wu

2023-07-07 08:11:32

by Xingyu Wu

Subject: Re: [RESEND PATCH v6 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC

On 2023/7/5 14:27, Krzysztof Kozlowski wrote:
> On 04/07/2023 08:46, Xingyu Wu wrote:
>> [Resending because it has a error about examples in syscon bingdings
>> and has to be fixed.]
>
> Resending means you sent the same version. If you did not change
> anything, then it will fail, right? If you changed something, it is new
> version.
>

Okay, I will fix it in new version of these patches. I sorry to make these noise.

Best regards,
Xingyu Wu

2023-07-12 03:43:06

by Hal Feng

Subject: Re: [RESEND PATCH v6 5/7] clk: starfive: jh7110-sys: Add PLL clocks source from DTS

On Tue, 4 Jul 2023 14:46:08 +0800, Xingyu Wu wrote:
> Modify PLL clocks source to be got from DTS or
> the fixed factor clocks.
>
> Signed-off-by: Xingyu Wu <[email protected]>

Reviewed-by: Hal Feng <[email protected]>

2023-07-12 16:22:52

by Conor Dooley

Subject: Re: [RESEND PATCH v6 0/7] Add PLL clocks driver and syscon for StarFive JH7110 SoC

Stephen,

On Tue, Jul 04, 2023 at 11:29:15PM +0100, Conor Dooley wrote:
> On Tue, Jul 04, 2023 at 02:46:03PM +0800, Xingyu Wu wrote:
> > [Resending because it has a error about examples in syscon bingdings
> > and has to be fixed.]
> >
> > This patch serises are to add PLL clocks driver and providers by writing
> > and reading syscon registers for the StarFive JH7110 RISC-V SoC. And add
> > documentation and nodes to describe StarFive System Controller(syscon)
> > Registers. This patch serises are based on Linux 6.4.
>
> Could you take a look at this series when you get a chance, please?
> Would be good to finally get it merged since the syscon bits are a dep
> for a few other things :)

When Emil has had a chance to look at this, my plan was to send you a PR
for the bindings & clock bits, like I did for the last round of StarFive
changes. Would that be okay with you?

Thanks,
Conor.

> > William Qiu (2):
> > dt-bindings: soc: starfive: Add StarFive syscon module
> > riscv: dts: starfive: jh7110: Add syscon nodes
> >
> > Xingyu Wu (5):
> > dt-bindings: clock: Add StarFive JH7110 PLL clock generator
> > dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs
> > clk: starfive: Add StarFive JH7110 PLL clock driver
> > clk: starfive: jh7110-sys: Add PLL clocks source from DTS
> > riscv: dts: starfive: jh7110: Add PLL clocks source in SYSCRG node
> >
> > .../bindings/clock/starfive,jh7110-pll.yaml | 46 ++
> > .../clock/starfive,jh7110-syscrg.yaml | 18 +-
> > .../soc/starfive/starfive,jh7110-syscon.yaml | 93 ++++
> > MAINTAINERS | 13 +
> > arch/riscv/boot/dts/starfive/jh7110.dtsi | 30 +-
> > drivers/clk/starfive/Kconfig | 9 +
> > drivers/clk/starfive/Makefile | 1 +
> > .../clk/starfive/clk-starfive-jh7110-pll.c | 507 ++++++++++++++++++
> > .../clk/starfive/clk-starfive-jh7110-sys.c | 45 +-
> > .../dt-bindings/clock/starfive,jh7110-crg.h | 6 +
> > 10 files changed, 746 insertions(+), 22 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
> > create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> > create mode 100644 drivers/clk/starfive/clk-starfive-jh7110-pll.c
> >
> > --
> > 2.25.1
> >

Attachments:

2023-07-13 12:47:56

by Emil Renner Berthing

Subject: Re: [RESEND PATCH v6 3/7] dt-bindings: clock: jh7110-syscrg: Add PLL clock inputs

On Tue, 4 Jul 2023 at 08:49, Xingyu Wu <[email protected]> wrote:
>
> Add PLL clock inputs from PLL clock generator.
>
> Signed-off-by: Xingyu Wu <[email protected]>

Reviewed-by: Emil Renner Berthing <[email protected]>

> ---
> .../bindings/clock/starfive,jh7110-syscrg.yaml | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> index 84373ae31644..5ba0a885aa80 100644
> --- a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> +++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> @@ -27,6 +27,9 @@ properties:
> - description: External I2S RX left/right channel clock
> - description: External TDM clock
> - description: External audio master clock
> + - description: PLL0
> + - description: PLL1
> + - description: PLL2
>
> - items:
> - description: Main Oscillator (24 MHz)
> @@ -38,6 +41,9 @@ properties:
> - description: External I2S RX left/right channel clock
> - description: External TDM clock
> - description: External audio master clock
> + - description: PLL0
> + - description: PLL1
> + - description: PLL2
>
> clock-names:
> oneOf:
> @@ -52,6 +58,9 @@ properties:
> - const: i2srx_lrck_ext
> - const: tdm_ext
> - const: mclk_ext
> + - const: pll0_out
> + - const: pll1_out
> + - const: pll2_out
>
> - items:
> - const: osc
> @@ -63,6 +72,9 @@ properties:
> - const: i2srx_lrck_ext
> - const: tdm_ext
> - const: mclk_ext
> + - const: pll0_out
> + - const: pll1_out
> + - const: pll2_out
>
> '#clock-cells':
> const: 1
> @@ -93,12 +105,14 @@ examples:
> <&gmac1_rgmii_rxin>,
> <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
> <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
> - <&tdm_ext>, <&mclk_ext>;
> + <&tdm_ext>, <&mclk_ext>,
> + <&pllclk 0>, <&pllclk 1>, <&pllclk 2>;
> clock-names = "osc", "gmac1_rmii_refin",
> "gmac1_rgmii_rxin",
> "i2stx_bclk_ext", "i2stx_lrck_ext",
> "i2srx_bclk_ext", "i2srx_lrck_ext",
> - "tdm_ext", "mclk_ext";
> + "tdm_ext", "mclk_ext",
> + "pll0_out", "pll1_out", "pll2_out";
> #clock-cells = <1>;
> #reset-cells = <1>;
> };
> --
> 2.25.1
>

2023-07-13 12:57:12

by Emil Renner Berthing

Subject: Re: [RESEND PATCH v6 1/7] dt-bindings: clock: Add StarFive JH7110 PLL clock generator

On Tue, 4 Jul 2023 at 08:49, Xingyu Wu <[email protected]> wrote:
>
> Add bindings for the PLL clock generator on the JH7110 RISC-V SoC.
>
> Reviewed-by: Conor Dooley <[email protected]>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> Signed-off-by: Xingyu Wu <[email protected]>
> ---
> .../bindings/clock/starfive,jh7110-pll.yaml | 46 +++++++++++++++++++
> .../dt-bindings/clock/starfive,jh7110-crg.h | 6 +++
> 2 files changed, 52 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
>
> diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
> new file mode 100644
> index 000000000000..beb78add5a8d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/starfive,jh7110-pll.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 PLL Clock Generator
> +
> +description:
> + These PLLs are high speed, low jitter frequency synthesizers in JH7110.

..synthesizers in the JH7110.

> + Each PLL works in integer mode or fraction mode, with configuration
> + registers in the sys syscon. So the PLLs node should be a child of
> + SYS-SYSCON node.
> + The formula for calculating frequency is
> + Fvco = Fref * (NI + NF) / M / Q1
> +
> +maintainers:
> + - Xingyu Wu <[email protected]>
> +
> +properties:
> + compatible:
> + const: starfive,jh7110-pll
> +
> + clocks:
> + maxItems: 1
> + description: Main Oscillator (24 MHz)
> +
> + '#clock-cells':
> + const: 1
> + description:
> + See <dt-bindings/clock/starfive,jh7110-crg.h> for valid indices.
> +
> +required:
> + - compatible
> + - clocks
> + - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + clock-controller {
> + compatible = "starfive,jh7110-pll";
> + clocks = <&osc>;
> + #clock-cells = <1>;
> + };
> diff --git a/include/dt-bindings/clock/starfive,jh7110-crg.h b/include/dt-bindings/clock/starfive,jh7110-crg.h
> index 06257bfd9ac1..086a6ddcf380 100644
> --- a/include/dt-bindings/clock/starfive,jh7110-crg.h
> +++ b/include/dt-bindings/clock/starfive,jh7110-crg.h
> @@ -6,6 +6,12 @@
> #ifndef __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
> #define __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
>
> +/* PLL clocks */
> +#define JH7110_CLK_PLL0_OUT 0
> +#define JH7110_CLK_PLL1_OUT 1
> +#define JH7110_CLK_PLL2_OUT 2
> +#define JH7110_PLLCLK_END 3

It would be nice if these names followed the same pattern as the
clocks below. Eg. something like JH7110_PLLCLK_PLL?_OUT and
JH7110_PLLCLK_END.

But maybe these defines are not even needed, since you just do <&pll
0>, <&pll 1> and it's obvious what that means.

> /* SYSCRG clocks */
> #define JH7110_SYSCLK_CPU_ROOT 0
> #define JH7110_SYSCLK_CPU_CORE 1
> --
> 2.25.1
>

2023-07-13 12:58:42

by Emil Renner Berthing

Subject: Re: [RESEND PATCH v6 4/7] clk: starfive: Add StarFive JH7110 PLL clock driver

On Tue, 4 Jul 2023 at 08:49, Xingyu Wu <[email protected]> wrote:
>
> Add driver for the StarFive JH7110 PLL clock controller
> and they work by reading and setting syscon registers.
>
> Co-developed-by: Emil Renner Berthing <[email protected]>
> Signed-off-by: Emil Renner Berthing <[email protected]>
> Signed-off-by: Xingyu Wu <[email protected]>

Thanks for using my changes! I'll add my r-b for Conor, but since I'm
also a co-developer it's fine to leave it out if you resubmit.
Reviewed-by: Emil Renner Berthing <[email protected]>

> ---
> MAINTAINERS | 6 +
> drivers/clk/starfive/Kconfig | 8 +
> drivers/clk/starfive/Makefile | 1 +
> .../clk/starfive/clk-starfive-jh7110-pll.c | 507 ++++++++++++++++++
> 4 files changed, 522 insertions(+)
> create mode 100644 drivers/clk/starfive/clk-starfive-jh7110-pll.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58ba04bd0bc8..b59b4d217991 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20127,6 +20127,12 @@ S: Supported
> F: Documentation/devicetree/bindings/mmc/starfive*
> F: drivers/mmc/host/dw_mmc-starfive.c
>
> +STARFIVE JH7110 PLL CLOCK DRIVER
> +M: Xingyu Wu <[email protected]>
> +S: Supported
> +F: Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
> +F: drivers/clk/starfive/clk-starfive-jh7110-pll.*
> +
> STARFIVE JH7110 SYSCON
> M: William Qiu <[email protected]>
> M: Xingyu Wu <[email protected]>
> diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
> index 5d2333106f13..5195f7be5213 100644
> --- a/drivers/clk/starfive/Kconfig
> +++ b/drivers/clk/starfive/Kconfig
> @@ -21,6 +21,14 @@ config CLK_STARFIVE_JH7100_AUDIO
> Say Y or M here to support the audio clocks on the StarFive JH7100
> SoC.
>
> +config CLK_STARFIVE_JH7110_PLL
> + bool "StarFive JH7110 PLL clock support"
> + depends on ARCH_STARFIVE || COMPILE_TEST
> + default ARCH_STARFIVE
> + help
> + Say yes here to support the PLL clock controller on the
> + StarFive JH7110 SoC.
> +
> config CLK_STARFIVE_JH7110_SYS
> bool "StarFive JH7110 system clock support"
> depends on ARCH_STARFIVE || COMPILE_TEST
> diff --git a/drivers/clk/starfive/Makefile b/drivers/clk/starfive/Makefile
> index f3df7d957b1e..b48e539e52b0 100644
> --- a/drivers/clk/starfive/Makefile
> +++ b/drivers/clk/starfive/Makefile
> @@ -4,5 +4,6 @@ obj-$(CONFIG_CLK_STARFIVE_JH71X0) += clk-starfive-jh71x0.o
> obj-$(CONFIG_CLK_STARFIVE_JH7100) += clk-starfive-jh7100.o
> obj-$(CONFIG_CLK_STARFIVE_JH7100_AUDIO) += clk-starfive-jh7100-audio.o
>
> +obj-$(CONFIG_CLK_STARFIVE_JH7110_PLL) += clk-starfive-jh7110-pll.o
> obj-$(CONFIG_CLK_STARFIVE_JH7110_SYS) += clk-starfive-jh7110-sys.o
> obj-$(CONFIG_CLK_STARFIVE_JH7110_AON) += clk-starfive-jh7110-aon.o
> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-pll.c b/drivers/clk/starfive/clk-starfive-jh7110-pll.c
> new file mode 100644
> index 000000000000..b10c142d456d
> --- /dev/null
> +++ b/drivers/clk/starfive/clk-starfive-jh7110-pll.c
> @@ -0,0 +1,507 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * StarFive JH7110 PLL Clock Generator Driver
> + *
> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> + * Copyright (C) 2023 Emil Renner Berthing <[email protected]>
> + *
> + * This driver is about to register JH7110 PLL clock generator and support ops.
> + * The JH7110 have three PLL clock, PLL0, PLL1 and PLL2.
> + * Each PLL clocks work in integer mode or fraction mode by some dividers,
> + * and the configuration registers and dividers are set in several syscon registers.
> + * The formula for calculating frequency is:
> + * Fvco = Fref * (NI + NF) / M / Q1
> + * Fref: OSC source clock rate
> + * NI: integer frequency dividing ratio of feedback divider, set by fbdiv[11:0].
> + * NF: fractional frequency dividing ratio, set by frac[23:0]. NF = frac[23:0] / 2^24 = 0 ~ 0.999.
> + * M: frequency dividing ratio of pre-divider, set by prediv[5:0].
> + * Q1: frequency dividing ratio of post divider, set by 2^postdiv1[1:0], eg. 1, 2, 4 or 8.
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk-provider.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
> +
> +/* this driver expects a 24MHz input frequency from the oscillator */
> +#define JH7110_PLL_OSC_RATE 24000000UL
> +
> +#define JH7110_PLL0_PD_OFFSET 0x18
> +#define JH7110_PLL0_DACPD_SHIFT 24
> +#define JH7110_PLL0_DACPD_MASK BIT(24)
> +#define JH7110_PLL0_DSMPD_SHIFT 25
> +#define JH7110_PLL0_DSMPD_MASK BIT(25)
> +#define JH7110_PLL0_FBDIV_OFFSET 0x1c
> +#define JH7110_PLL0_FBDIV_SHIFT 0
> +#define JH7110_PLL0_FBDIV_MASK GENMASK(11, 0)
> +#define JH7110_PLL0_FRAC_OFFSET 0x20
> +#define JH7110_PLL0_PREDIV_OFFSET 0x24
> +
> +#define JH7110_PLL1_PD_OFFSET 0x24
> +#define JH7110_PLL1_DACPD_SHIFT 15
> +#define JH7110_PLL1_DACPD_MASK BIT(15)
> +#define JH7110_PLL1_DSMPD_SHIFT 16
> +#define JH7110_PLL1_DSMPD_MASK BIT(16)
> +#define JH7110_PLL1_FBDIV_OFFSET 0x24
> +#define JH7110_PLL1_FBDIV_SHIFT 17
> +#define JH7110_PLL1_FBDIV_MASK GENMASK(28, 17)
> +#define JH7110_PLL1_FRAC_OFFSET 0x28
> +#define JH7110_PLL1_PREDIV_OFFSET 0x2c
> +
> +#define JH7110_PLL2_PD_OFFSET 0x2c
> +#define JH7110_PLL2_DACPD_SHIFT 15
> +#define JH7110_PLL2_DACPD_MASK BIT(15)
> +#define JH7110_PLL2_DSMPD_SHIFT 16
> +#define JH7110_PLL2_DSMPD_MASK BIT(16)
> +#define JH7110_PLL2_FBDIV_OFFSET 0x2c
> +#define JH7110_PLL2_FBDIV_SHIFT 17
> +#define JH7110_PLL2_FBDIV_MASK GENMASK(28, 17)
> +#define JH7110_PLL2_FRAC_OFFSET 0x30
> +#define JH7110_PLL2_PREDIV_OFFSET 0x34
> +
> +#define JH7110_PLL_FRAC_SHIFT 0
> +#define JH7110_PLL_FRAC_MASK GENMASK(23, 0)
> +#define JH7110_PLL_POSTDIV1_SHIFT 28
> +#define JH7110_PLL_POSTDIV1_MASK GENMASK(29, 28)
> +#define JH7110_PLL_PREDIV_SHIFT 0
> +#define JH7110_PLL_PREDIV_MASK GENMASK(5, 0)
> +
> +enum jh7110_pll_mode {
> + JH7110_PLL_MODE_FRACTION,
> + JH7110_PLL_MODE_INTEGER,
> +};
> +
> +struct jh7110_pll_preset {
> + unsigned long freq;
> + u32 frac; /* frac value should be decimals multiplied by 2^24 */
> + unsigned fbdiv : 12; /* fbdiv value should be 8 to 4095 */
> + unsigned prediv : 6;
> + unsigned postdiv1 : 2;
> + unsigned mode : 1;
> +};
> +
> +struct jh7110_pll_info {
> + char *name;
> + const struct jh7110_pll_preset *presets;
> + unsigned int npresets;
> + struct {
> + unsigned int pd;
> + unsigned int fbdiv;
> + unsigned int frac;
> + unsigned int prediv;
> + } offsets;
> + struct {
> + u32 dacpd;
> + u32 dsmpd;
> + u32 fbdiv;
> + } masks;
> + struct {
> + char dacpd;
> + char dsmpd;
> + char fbdiv;
> + } shifts;
> +};
> +
> +#define _JH7110_PLL(_idx, _name, _presets) \
> + [_idx] = { \
> + .name = _name, \
> + .presets = _presets, \
> + .npresets = ARRAY_SIZE(_presets), \
> + .offsets = { \
> + .pd = JH7110_PLL##_idx##_PD_OFFSET, \
> + .fbdiv = JH7110_PLL##_idx##_FBDIV_OFFSET, \
> + .frac = JH7110_PLL##_idx##_FRAC_OFFSET, \
> + .prediv = JH7110_PLL##_idx##_PREDIV_OFFSET, \
> + }, \
> + .masks = { \
> + .dacpd = JH7110_PLL##_idx##_DACPD_MASK, \
> + .dsmpd = JH7110_PLL##_idx##_DSMPD_MASK, \
> + .fbdiv = JH7110_PLL##_idx##_FBDIV_MASK, \
> + }, \
> + .shifts = { \
> + .dacpd = JH7110_PLL##_idx##_DACPD_SHIFT, \
> + .dsmpd = JH7110_PLL##_idx##_DSMPD_SHIFT, \
> + .fbdiv = JH7110_PLL##_idx##_FBDIV_SHIFT, \
> + }, \
> + }
> +#define JH7110_PLL(idx, name, presets) _JH7110_PLL(idx, name, presets)
> +
> +struct jh7110_pll_data {
> + struct clk_hw hw;
> + unsigned int idx;
> +};
> +
> +struct jh7110_pll_priv {
> + struct device *dev;
> + struct regmap *regmap;
> + struct jh7110_pll_data pll[JH7110_PLLCLK_END];
> +};
> +
> +struct jh7110_pll_regvals {
> + u32 dacpd;
> + u32 dsmpd;
> + u32 fbdiv;
> + u32 frac;
> + u32 postdiv1;
> + u32 prediv;
> +};
> +
> +/*
> + * Because the pll frequency is relatively fixed,
> + * it cannot be set arbitrarily, so it needs a specific configuration.
> + * PLL0 frequency should be multiple of 125MHz (USB frequency).
> + */
> +static const struct jh7110_pll_preset jh7110_pll0_presets[] = {
> + {
> + .freq = 375000000,
> + .fbdiv = 125,
> + .prediv = 8,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 500000000,
> + .fbdiv = 125,
> + .prediv = 6,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 625000000,
> + .fbdiv = 625,
> + .prediv = 24,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 750000000,
> + .fbdiv = 125,
> + .prediv = 4,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 875000000,
> + .fbdiv = 875,
> + .prediv = 24,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 1000000000,
> + .fbdiv = 125,
> + .prediv = 3,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 1250000000,
> + .fbdiv = 625,
> + .prediv = 12,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 1375000000,
> + .fbdiv = 1375,
> + .prediv = 24,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 1500000000,
> + .fbdiv = 125,
> + .prediv = 2,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + },
> +};
> +
> +static const struct jh7110_pll_preset jh7110_pll1_presets[] = {
> + {
> + .freq = 1066000000,
> + .fbdiv = 533,
> + .prediv = 12,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 1200000000,
> + .fbdiv = 50,
> + .prediv = 1,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 1400000000,
> + .fbdiv = 350,
> + .prediv = 6,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 1600000000,
> + .fbdiv = 200,
> + .prediv = 3,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + },
> +};
> +
> +static const struct jh7110_pll_preset jh7110_pll2_presets[] = {
> + {
> + .freq = 1188000000,
> + .fbdiv = 99,
> + .prediv = 2,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + }, {
> + .freq = 1228800000,
> + .fbdiv = 256,
> + .prediv = 5,
> + .postdiv1 = 0,
> + .mode = JH7110_PLL_MODE_INTEGER,
> + },
> +};
> +
> +static const struct jh7110_pll_info jh7110_plls[JH7110_PLLCLK_END] = {
> + JH7110_PLL(JH7110_CLK_PLL0_OUT, "pll0_out", jh7110_pll0_presets),
> + JH7110_PLL(JH7110_CLK_PLL1_OUT, "pll1_out", jh7110_pll1_presets),
> + JH7110_PLL(JH7110_CLK_PLL2_OUT, "pll2_out", jh7110_pll2_presets),
> +};
> +
> +static struct jh7110_pll_data *jh7110_pll_data_from(struct clk_hw *hw)
> +{
> + return container_of(hw, struct jh7110_pll_data, hw);
> +}
> +
> +static struct jh7110_pll_priv *jh7110_pll_priv_from(struct jh7110_pll_data *pll)
> +{
> + return container_of(pll, struct jh7110_pll_priv, pll[pll->idx]);
> +}
> +
> +static void jh7110_pll_regvals_get(struct regmap *regmap,
> + const struct jh7110_pll_info *info,
> + struct jh7110_pll_regvals *ret)
> +{
> + u32 val;
> +
> + regmap_read(regmap, info->offsets.pd, &val);
> + ret->dacpd = (val & info->masks.dacpd) >> info->shifts.dacpd;
> + ret->dsmpd = (val & info->masks.dsmpd) >> info->shifts.dsmpd;
> +
> + regmap_read(regmap, info->offsets.fbdiv, &val);
> + ret->fbdiv = (val & info->masks.fbdiv) >> info->shifts.fbdiv;
> +
> + regmap_read(regmap, info->offsets.frac, &val);
> + ret->frac = (val & JH7110_PLL_FRAC_MASK) >> JH7110_PLL_FRAC_SHIFT;
> + ret->postdiv1 = (val & JH7110_PLL_POSTDIV1_MASK) >> JH7110_PLL_POSTDIV1_SHIFT;
> +
> + regmap_read(regmap, info->offsets.prediv, &val);
> + ret->prediv = (val & JH7110_PLL_PREDIV_MASK) >> JH7110_PLL_PREDIV_SHIFT;
> +}
> +
> +static unsigned long jh7110_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> +{
> + struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
> + struct jh7110_pll_priv *priv = jh7110_pll_priv_from(pll);
> + struct jh7110_pll_regvals val;
> + unsigned long rate;
> +
> + jh7110_pll_regvals_get(priv->regmap, &jh7110_plls[pll->idx], &val);
> +
> + /*
> + * dacpd = dsmpd = 0: fraction mode
> + * dacpd = dsmpd = 1: integer mode, frac value ignored
> + *
> + * rate = parent * (fbdiv + frac/2^24) / prediv / 2^postdiv1
> + * = (parent * fbdiv + parent * frac / 2^24) / (prediv * 2^postdiv1)
> + */
> + if (val.dacpd == 0 && val.dsmpd == 0)
> + rate = parent_rate * val.frac / (1UL << 24);
> + else if (val.dacpd == 1 && val.dsmpd == 1)
> + rate = 0;
> + else
> + return 0;
> +
> + rate += parent_rate * val.fbdiv;
> + rate /= val.prediv << val.postdiv1;
> +
> + return rate;
> +}
> +
> +static int jh7110_pll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> +{
> + struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
> + const struct jh7110_pll_info *info = &jh7110_plls[pll->idx];
> + const struct jh7110_pll_preset *selected = &info->presets[0];
> + unsigned int idx;
> +
> + /* if the parent rate doesn't match our expectations the presets won't work */
> + if (req->best_parent_rate != JH7110_PLL_OSC_RATE) {
> + req->rate = jh7110_pll_recalc_rate(hw, req->best_parent_rate);
> + return 0;
> + }
> +
> + /* find highest rate lower or equal to the requested rate */
> + for (idx = 1; idx < info->npresets; idx++) {
> + const struct jh7110_pll_preset *val = &info->presets[idx];
> +
> + if (req->rate < val->freq)
> + break;
> +
> + selected = val;
> + }
> +
> + req->rate = selected->freq;
> + return 0;
> +}
> +
> +static int jh7110_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
> + struct jh7110_pll_priv *priv = jh7110_pll_priv_from(pll);
> + const struct jh7110_pll_info *info = &jh7110_plls[pll->idx];
> + const struct jh7110_pll_preset *val;
> + unsigned int idx;
> +
> + /* if the parent rate doesn't match our expectations the presets won't work */
> + if (parent_rate != JH7110_PLL_OSC_RATE)
> + return -EINVAL;
> +
> + for (idx = 0, val = &info->presets[0]; idx < info->npresets; idx++, val++) {
> + if (val->freq == rate)
> + goto found;
> + }
> + return -EINVAL;
> +
> +found:
> + if (val->mode == JH7110_PLL_MODE_FRACTION)
> + regmap_update_bits(priv->regmap, info->offsets.frac, JH7110_PLL_FRAC_MASK,
> + val->frac << JH7110_PLL_FRAC_SHIFT);
> +
> + regmap_update_bits(priv->regmap, info->offsets.pd, info->masks.dacpd,
> + (u32)val->mode << info->shifts.dacpd);
> + regmap_update_bits(priv->regmap, info->offsets.pd, info->masks.dsmpd,
> + (u32)val->mode << info->shifts.dsmpd);
> + regmap_update_bits(priv->regmap, info->offsets.prediv, JH7110_PLL_PREDIV_MASK,
> + (u32)val->prediv << JH7110_PLL_PREDIV_SHIFT);
> + regmap_update_bits(priv->regmap, info->offsets.fbdiv, info->masks.fbdiv,
> + val->fbdiv << info->shifts.fbdiv);
> + regmap_update_bits(priv->regmap, info->offsets.frac, JH7110_PLL_POSTDIV1_MASK,
> + (u32)val->postdiv1 << JH7110_PLL_POSTDIV1_SHIFT);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_DEBUG_FS
> +static int jh7110_pll_registers_read(struct seq_file *s, void *unused)
> +{
> + struct jh7110_pll_data *pll = s->private;
> + struct jh7110_pll_priv *priv = jh7110_pll_priv_from(pll);
> + struct jh7110_pll_regvals val;
> +
> + jh7110_pll_regvals_get(priv->regmap, &jh7110_plls[pll->idx], &val);
> +
> + seq_printf(s, "fbdiv=%u\n"
> + "frac=%u\n"
> + "prediv=%u\n"
> + "postdiv1=%u\n"
> + "dacpd=%u\n"
> + "dsmpd=%u\n",
> + val.fbdiv, val.frac, val.prediv, val.postdiv1,
> + val.dacpd, val.dsmpd);
> +
> + return 0;
> +}
> +
> +static int jh7110_pll_registers_open(struct inode *inode, struct file *f)
> +{
> + return single_open(f, jh7110_pll_registers_read, inode->i_private);
> +}
> +
> +static const struct file_operations jh7110_pll_registers_ops = {
> + .owner = THIS_MODULE,
> + .open = jh7110_pll_registers_open,
> + .release = single_release,
> + .read = seq_read,
> + .llseek = seq_lseek
> +};
> +
> +static void jh7110_pll_debug_init(struct clk_hw *hw, struct dentry *dentry)
> +{
> + struct jh7110_pll_data *pll = jh7110_pll_data_from(hw);
> +
> + debugfs_create_file("registers", 0400, dentry, pll,
> + &jh7110_pll_registers_ops);
> +}
> +#else
> +#define jh7110_pll_debug_init NULL
> +#endif
> +
> +static const struct clk_ops jh7110_pll_ops = {
> + .recalc_rate = jh7110_pll_recalc_rate,
> + .determine_rate = jh7110_pll_determine_rate,
> + .set_rate = jh7110_pll_set_rate,
> + .debug_init = jh7110_pll_debug_init,
> +};
> +
> +static struct clk_hw *jh7110_pll_get(struct of_phandle_args *clkspec, void *data)
> +{
> + struct jh7110_pll_priv *priv = data;
> + unsigned int idx = clkspec->args[0];
> +
> + if (idx < JH7110_PLLCLK_END)
> + return &priv->pll[idx].hw;
> +
> + return ERR_PTR(-EINVAL);
> +}
> +
> +static int jh7110_pll_probe(struct platform_device *pdev)
> +{
> + struct jh7110_pll_priv *priv;
> + unsigned int idx;
> + int ret;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->dev = &pdev->dev;
> + priv->regmap = syscon_node_to_regmap(priv->dev->of_node->parent);
> + if (IS_ERR(priv->regmap))
> + return PTR_ERR(priv->regmap);
> +
> + for (idx = 0; idx < JH7110_PLLCLK_END; idx++) {
> + struct clk_parent_data parents = {
> + .index = 0,
> + };
> + struct clk_init_data init = {
> + .name = jh7110_plls[idx].name,
> + .ops = &jh7110_pll_ops,
> + .parent_data = &parents,
> + .num_parents = 1,
> + .flags = 0,
> + };
> + struct jh7110_pll_data *pll = &priv->pll[idx];
> +
> + pll->hw.init = &init;
> + pll->idx = idx;
> +
> + ret = devm_clk_hw_register(&pdev->dev, &pll->hw);
> + if (ret)
> + return ret;
> + }
> +
> + return devm_of_clk_add_hw_provider(&pdev->dev, jh7110_pll_get, priv);
> +}
> +
> +static const struct of_device_id jh7110_pll_match[] = {
> + { .compatible = "starfive,jh7110-pll" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, jh7110_pll_match);
> +
> +static struct platform_driver jh7110_pll_driver = {
> + .driver = {
> + .name = "clk-starfive-jh7110-pll",
> + .of_match_table = jh7110_pll_match,
> + },
> +};
> +builtin_platform_driver_probe(jh7110_pll_driver, jh7110_pll_probe);
> --
> 2.25.1
>

2023-07-13 13:00:42

by Emil Renner Berthing

Subject: Re: [RESEND PATCH v6 2/7] dt-bindings: soc: starfive: Add StarFive syscon module

On Tue, 4 Jul 2023 at 08:49, Xingyu Wu <[email protected]> wrote:
>
> From: William Qiu <[email protected]>
>
> Add documentation to describe StarFive System Controller Registers.
>

Reviewed-by: Emil Renner Berthing <[email protected]>

> Co-developed-by: Xingyu Wu <[email protected]>
> Signed-off-by: Xingyu Wu <[email protected]>
> Signed-off-by: William Qiu <[email protected]>
> ---
> .../soc/starfive/starfive,jh7110-syscon.yaml | 93 +++++++++++++++++++
> MAINTAINERS | 7 ++
> 2 files changed, 100 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
>
> diff --git a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> new file mode 100644
> index 000000000000..0039319e91fe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/starfive/starfive,jh7110-syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 SoC system controller
> +
> +maintainers:
> + - William Qiu <[email protected]>
> +
> +description:
> + The StarFive JH7110 SoC system controller provides register information such
> + as offset, mask and shift to configure related modules such as MMC and PCIe.
> +
> +properties:
> + compatible:
> + oneOf:
> + - items:
> + - const: starfive,jh7110-sys-syscon
> + - const: syscon
> + - const: simple-mfd
> + - items:
> + - enum:
> + - starfive,jh7110-aon-syscon
> + - starfive,jh7110-stg-syscon
> + - const: syscon
> +
> + reg:
> + maxItems: 1
> +
> + clock-controller:
> + $ref: /schemas/clock/starfive,jh7110-pll.yaml#
> + type: object
> +
> + "#power-domain-cells":
> + const: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: starfive,jh7110-sys-syscon
> + then:
> + required:
> + - clock-controller
> + else:
> + properties:
> + clock-controller: false
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: starfive,jh7110-aon-syscon
> + then:
> + required:
> + - "#power-domain-cells"
> + else:
> + properties:
> + "#power-domain-cells": false
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + syscon@10240000 {
> + compatible = "starfive,jh7110-stg-syscon", "syscon";
> + reg = <0x10240000 0x1000>;
> + };
> +
> + syscon@13030000 {
> + compatible = "starfive,jh7110-sys-syscon", "syscon", "simple-mfd";
> + reg = <0x13030000 0x1000>;
> +
> + clock-controller {
> + compatible = "starfive,jh7110-pll";
> + clocks = <&osc>;
> + #clock-cells = <1>;
> + };
> + };
> +
> + syscon@17010000 {
> + compatible = "starfive,jh7110-aon-syscon", "syscon";
> + reg = <0x17010000 0x1000>;
> + #power-domain-cells = <1>;
> + };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 35e19594640d..58ba04bd0bc8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20127,6 +20127,12 @@ S: Supported
> F: Documentation/devicetree/bindings/mmc/starfive*
> F: drivers/mmc/host/dw_mmc-starfive.c
>
> +STARFIVE JH7110 SYSCON
> +M: William Qiu <[email protected]>
> +M: Xingyu Wu <[email protected]>
> +S: Supported
> +F: Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> +
> STARFIVE JH71X0 CLOCK DRIVERS
> M: Emil Renner Berthing <[email protected]>
> M: Hal Feng <[email protected]>
> @@ -20164,6 +20170,7 @@ STARFIVE SOC DRIVERS
> M: Conor Dooley <[email protected]>
> S: Maintained
> T: git https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/
> +F: Documentation/devicetree/bindings/soc/starfive/
> F: drivers/soc/starfive/
>
> STARFIVE TRNG DRIVER
> --
> 2.25.1
>

2023-07-13 13:37:53

by Emil Renner Berthing

Subject: Re: [RESEND PATCH v6 7/7] riscv: dts: starfive: jh7110: Add PLL clocks source in SYSCRG node

On Tue, 4 Jul 2023 at 08:49, Xingyu Wu <[email protected]> wrote:
>
> Add PLL clocks input from PLL clocks driver in SYSCRG node.
>
> Reviewed-by: Conor Dooley <[email protected]>
> Signed-off-by: Xingyu Wu <[email protected]>
> ---
> arch/riscv/boot/dts/starfive/jh7110.dtsi | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index 11dd4c9d64b0..cdfd036a0e6c 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -452,12 +452,16 @@ syscrg: clock-controller@13020000 {
> <&gmac1_rgmii_rxin>,
> <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
> <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
> - <&tdm_ext>, <&mclk_ext>;
> + <&tdm_ext>, <&mclk_ext>,
> + <&pllclk JH7110_CLK_PLL0_OUT>,
> + <&pllclk JH7110_CLK_PLL1_OUT>,
> + <&pllclk JH7110_CLK_PLL2_OUT>;

Once these are updated to <&pll ?> or <&pllclk JH7110_PLLCLK_PLL?_OUT>
if you still want to keep the defines:
Reviewed-by: Emil Renner Berthing <[email protected]>

> clock-names = "osc", "gmac1_rmii_refin",
> "gmac1_rgmii_rxin",
> "i2stx_bclk_ext", "i2stx_lrck_ext",
> "i2srx_bclk_ext", "i2srx_lrck_ext",
> - "tdm_ext", "mclk_ext";
> + "tdm_ext", "mclk_ext",
> + "pll0_out", "pll1_out", "pll2_out";
> #clock-cells = <1>;
> #reset-cells = <1>;
> };
> --
> 2.25.1
>

2023-07-13 13:40:38

by Emil Renner Berthing

Subject: Re: [RESEND PATCH v6 6/7] riscv: dts: starfive: jh7110: Add syscon nodes

On Tue, 4 Jul 2023 at 08:49, Xingyu Wu <[email protected]> wrote:
>
> From: William Qiu <[email protected]>
>
> Add stg_syscon/sys_syscon/aon_syscon/PLL nodes for JH7110 SoC.
>
> Reviewed-by: Conor Dooley <[email protected]>
> Co-developed-by: Xingyu Wu <[email protected]>
> Signed-off-by: Xingyu Wu <[email protected]>
> Signed-off-by: William Qiu <[email protected]>
> ---
> arch/riscv/boot/dts/starfive/jh7110.dtsi | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index 4c5fdb905da8..11dd4c9d64b0 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -353,6 +353,11 @@ i2c2: i2c@10050000 {
> status = "disabled";
> };
>
> + stg_syscon: syscon@10240000 {
> + compatible = "starfive,jh7110-stg-syscon", "syscon";
> + reg = <0x0 0x10240000 0x0 0x1000>;
> + };
> +
> uart3: serial@12000000 {
> compatible = "snps,dw-apb-uart";
> reg = <0x0 0x12000000 0x0 0x10000>;
> @@ -457,6 +462,17 @@ syscrg: clock-controller@13020000 {
> #reset-cells = <1>;
> };
>
> + sys_syscon: syscon@13030000 {
> + compatible = "starfive,jh7110-sys-syscon", "syscon", "simple-mfd";
> + reg = <0x0 0x13030000 0x0 0x1000>;
> +
> + pllclk: clock-controller {

Maybe call the handle "pll", so the references can be just <&pll 0>,
<&pll 1> and <&pll 2> if you choose to drop the JH7110_PLLCLK_PLL?_OUT
defines.

In any case:
Reviewed-by: Emil Renner Berthing <[email protected]>

> + compatible = "starfive,jh7110-pll";
> + clocks = <&osc>;
> + #clock-cells = <1>;
> + };
> + };
> +
> sysgpio: pinctrl@13040000 {
> compatible = "starfive,jh7110-sys-pinctrl";
> reg = <0x0 0x13040000 0x0 0x10000>;
> @@ -486,6 +502,12 @@ aoncrg: clock-controller@17000000 {
> #reset-cells = <1>;
> };
>
> + aon_syscon: syscon@17010000 {
> + compatible = "starfive,jh7110-aon-syscon", "syscon";
> + reg = <0x0 0x17010000 0x0 0x1000>;
> + #power-domain-cells = <1>;
> + };
> +
> aongpio: pinctrl@17020000 {
> compatible = "starfive,jh7110-aon-pinctrl";
> reg = <0x0 0x17020000 0x0 0x10000>;
> --
> 2.25.1
>

2023-07-13 13:42:48

by Emil Renner Berthing

Subject: Re: [RESEND PATCH v6 5/7] clk: starfive: jh7110-sys: Add PLL clocks source from DTS

On Tue, 4 Jul 2023 at 08:49, Xingyu Wu <[email protected]> wrote:
>
> Modify PLL clocks source to be got from DTS or
> the fixed factor clocks.
>
> Signed-off-by: Xingyu Wu <[email protected]>
> ---
> drivers/clk/starfive/Kconfig | 1 +
> .../clk/starfive/clk-starfive-jh7110-sys.c | 45 +++++++++++--------
> 2 files changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
> index 5195f7be5213..978b78ec08b1 100644
> --- a/drivers/clk/starfive/Kconfig
> +++ b/drivers/clk/starfive/Kconfig
> @@ -35,6 +35,7 @@ config CLK_STARFIVE_JH7110_SYS
> select AUXILIARY_BUS
> select CLK_STARFIVE_JH71X0
> select RESET_STARFIVE_JH7110 if RESET_CONTROLLER
> + select CLK_STARFIVE_JH7110_PLL
> default ARCH_STARFIVE
> help
> Say yes here to support the system clock controller on the
> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> index e6031345ef05..d56f48013388 100644
> --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/auxiliary_bus.h>
> +#include <linux/clk.h>
> #include <linux/clk-provider.h>
> #include <linux/init.h>
> #include <linux/io.h>
> @@ -386,6 +387,7 @@ EXPORT_SYMBOL_GPL(jh7110_reset_controller_register);
>
> static int __init jh7110_syscrg_probe(struct platform_device *pdev)
> {
> + bool use_fixed_pll = true; /* PLL clocks use fixed factor clocks or PLL driver */

nit: reverse christmas tree ordering, eg. move this below priv

> struct jh71x0_clk_priv *priv;
> unsigned int idx;
> int ret;
> @@ -402,28 +404,29 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
> if (IS_ERR(priv->base))
> return PTR_ERR(priv->base);
>
> - /*
> - * These PLL clocks are not actually fixed factor clocks and can be
> - * controlled by the syscon registers of JH7110. They will be dropped
> - * and registered in the PLL clock driver instead.
> - */
> + if (!IS_ERR(devm_clk_get(priv->dev, "pll0_out")))
> + use_fixed_pll = false; /* can get pll clocks from PLL driver */

The devm_clk_get() variant will allocate memory for a callback to call
clk_put() when the driver is unloaded, but proper references
associated with the consumers of the pll0_out clock are already taken
below. So unless we find a better way to detect if the pll references
are specified in the device tree or not, maybe something like this
instead:

priv->pll[0] = clk_get(priv->dev, "pll0_out);
if (IS_ERR(priv->pll[0])) {
/* 24MHZ -> 1000.0MHz */
priv->pll[0] = ...
...

} else {
clk_put(priv->pll[0]);
priv->pll[0] = NULL;
}

> + /* Use fixed factor clocks if can not get the PLL clocks from DTS */
> + if (use_fixed_pll) {
> /* 24MHz -> 1000.0MHz */

These comments are not indented with the code, which just looks weird.

> - priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
> - "osc", 0, 125, 3);
> - if (IS_ERR(priv->pll[0]))
> - return PTR_ERR(priv->pll[0]);
> + priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
> + "osc", 0, 125, 3);
> + if (IS_ERR(priv->pll[0]))
> + return PTR_ERR(priv->pll[0]);
>
> /* 24MHz -> 1066.0MHz */
> - priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
> - "osc", 0, 533, 12);
> - if (IS_ERR(priv->pll[1]))
> - return PTR_ERR(priv->pll[1]);
> + priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
> + "osc", 0, 533, 12);
> + if (IS_ERR(priv->pll[1]))
> + return PTR_ERR(priv->pll[1]);
>
> /* 24MHz -> 1188.0MHz */
> - priv->pll[2] = devm_clk_hw_register_fixed_factor(priv->dev, "pll2_out",
> - "osc", 0, 99, 2);
> - if (IS_ERR(priv->pll[2]))
> - return PTR_ERR(priv->pll[2]);
> + priv->pll[2] = devm_clk_hw_register_fixed_factor(priv->dev, "pll2_out",
> + "osc", 0, 99, 2);
> + if (IS_ERR(priv->pll[2]))
> + return PTR_ERR(priv->pll[2]);
> + }
>
> for (idx = 0; idx < JH7110_SYSCLK_END; idx++) {
> u32 max = jh7110_sysclk_data[idx].max;
> @@ -462,8 +465,14 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
> parents[i].fw_name = "tdm_ext";
> else if (pidx == JH7110_SYSCLK_MCLK_EXT)
> parents[i].fw_name = "mclk_ext";
> - else
> + else if (use_fixed_pll)

else if (priv->pll[0])

> parents[i].hw = priv->pll[pidx - JH7110_SYSCLK_PLL0_OUT];
> + else if (pidx == JH7110_SYSCLK_PLL0_OUT)
> + parents[i].fw_name = "pll0_out";
> + else if (pidx == JH7110_SYSCLK_PLL1_OUT)
> + parents[i].fw_name = "pll1_out";
> + else if (pidx == JH7110_SYSCLK_PLL2_OUT)
> + parents[i].fw_name = "pll2_out";
> }
>
> clk->hw.init = &init;
> --
> 2.25.1
>

2023-07-14 06:39:30

by Xingyu Wu

Subject: Re: [RESEND PATCH v6 1/7] dt-bindings: clock: Add StarFive JH7110 PLL clock generator

On 2023/7/13 20:26, Emil Renner Berthing wrote:
> On Tue, 4 Jul 2023 at 08:49, Xingyu Wu <[email protected]> wrote:
>>
>> Add bindings for the PLL clock generator on the JH7110 RISC-V SoC.
>>
>> Reviewed-by: Conor Dooley <[email protected]>
>> Reviewed-by: Krzysztof Kozlowski <[email protected]>
>> Signed-off-by: Xingyu Wu <[email protected]>
>> ---
>> .../bindings/clock/starfive,jh7110-pll.yaml | 46 +++++++++++++++++++
>> .../dt-bindings/clock/starfive,jh7110-crg.h | 6 +++
>> 2 files changed, 52 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
>> new file mode 100644
>> index 000000000000..beb78add5a8d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml
>> @@ -0,0 +1,46 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/starfive,jh7110-pll.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive JH7110 PLL Clock Generator
>> +
>> +description:
>> + These PLLs are high speed, low jitter frequency synthesizers in JH7110.
>
> ..synthesizers in the JH7110.

Will fix.

>
>> + Each PLL works in integer mode or fraction mode, with configuration
>> + registers in the sys syscon. So the PLLs node should be a child of
>> + SYS-SYSCON node.
>> + The formula for calculating frequency is
>> + Fvco = Fref * (NI + NF) / M / Q1
>> +
>> +maintainers:
>> + - Xingyu Wu <[email protected]>
>> +
>> +properties:
>> + compatible:
>> + const: starfive,jh7110-pll
>> +
>> + clocks:
>> + maxItems: 1
>> + description: Main Oscillator (24 MHz)
>> +
>> + '#clock-cells':
>> + const: 1
>> + description:
>> + See <dt-bindings/clock/starfive,jh7110-crg.h> for valid indices.
>> +
>> +required:
>> + - compatible
>> + - clocks
>> + - '#clock-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + clock-controller {
>> + compatible = "starfive,jh7110-pll";
>> + clocks = <&osc>;
>> + #clock-cells = <1>;
>> + };
>> diff --git a/include/dt-bindings/clock/starfive,jh7110-crg.h b/include/dt-bindings/clock/starfive,jh7110-crg.h
>> index 06257bfd9ac1..086a6ddcf380 100644
>> --- a/include/dt-bindings/clock/starfive,jh7110-crg.h
>> +++ b/include/dt-bindings/clock/starfive,jh7110-crg.h
>> @@ -6,6 +6,12 @@
>> #ifndef __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
>> #define __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
>>
>> +/* PLL clocks */
>> +#define JH7110_CLK_PLL0_OUT 0
>> +#define JH7110_CLK_PLL1_OUT 1
>> +#define JH7110_CLK_PLL2_OUT 2
>> +#define JH7110_PLLCLK_END 3
>
> It would be nice if these names followed the same pattern as the
> clocks below. Eg. something like JH7110_PLLCLK_PLL?_OUT and
> JH7110_PLLCLK_END.
>
> But maybe these defines are not even needed, since you just do <&pll
> 0>, <&pll 1> and it's obvious what that means.

I prefer to keep these names because they are used in the PLL driver
and are more easy to understand than numbers.
I will use the JH7110_PLLCLK_PLL?_OUT to follow the same pattern
in next version.

Best regards,
Xingyu Wu

>
>> /* SYSCRG clocks */
>> #define JH7110_SYSCLK_CPU_ROOT 0
>> #define JH7110_SYSCLK_CPU_CORE 1
>> --
>> 2.25.1
>>

2023-07-14 08:10:31

by Xingyu Wu

Subject: Re: [RESEND PATCH v6 5/7] clk: starfive: jh7110-sys: Add PLL clocks source from DTS

On 2023/7/13 21:15, Emil Renner Berthing wrote:
> On Tue, 4 Jul 2023 at 08:49, Xingyu Wu <[email protected]> wrote:
>>
>> Modify PLL clocks source to be got from DTS or
>> the fixed factor clocks.
>>
>> Signed-off-by: Xingyu Wu <[email protected]>
>> ---
>> drivers/clk/starfive/Kconfig | 1 +
>> .../clk/starfive/clk-starfive-jh7110-sys.c | 45 +++++++++++--------
>> 2 files changed, 28 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
>> index 5195f7be5213..978b78ec08b1 100644
>> --- a/drivers/clk/starfive/Kconfig
>> +++ b/drivers/clk/starfive/Kconfig
>> @@ -35,6 +35,7 @@ config CLK_STARFIVE_JH7110_SYS
>> select AUXILIARY_BUS
>> select CLK_STARFIVE_JH71X0
>> select RESET_STARFIVE_JH7110 if RESET_CONTROLLER
>> + select CLK_STARFIVE_JH7110_PLL
>> default ARCH_STARFIVE
>> help
>> Say yes here to support the system clock controller on the
>> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
>> index e6031345ef05..d56f48013388 100644
>> --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
>> +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
>> @@ -7,6 +7,7 @@
>> */
>>
>> #include <linux/auxiliary_bus.h>
>> +#include <linux/clk.h>
>> #include <linux/clk-provider.h>
>> #include <linux/init.h>
>> #include <linux/io.h>
>> @@ -386,6 +387,7 @@ EXPORT_SYMBOL_GPL(jh7110_reset_controller_register);
>>
>> static int __init jh7110_syscrg_probe(struct platform_device *pdev)
>> {
>> + bool use_fixed_pll = true; /* PLL clocks use fixed factor clocks or PLL driver */
>
> nit: reverse christmas tree ordering, eg. move this below priv
>
>> struct jh71x0_clk_priv *priv;
>> unsigned int idx;
>> int ret;
>> @@ -402,28 +404,29 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
>> if (IS_ERR(priv->base))
>> return PTR_ERR(priv->base);
>>
>> - /*
>> - * These PLL clocks are not actually fixed factor clocks and can be
>> - * controlled by the syscon registers of JH7110. They will be dropped
>> - * and registered in the PLL clock driver instead.
>> - */
>> + if (!IS_ERR(devm_clk_get(priv->dev, "pll0_out")))
>> + use_fixed_pll = false; /* can get pll clocks from PLL driver */
>
> The devm_clk_get() variant will allocate memory for a callback to call
> clk_put() when the driver is unloaded, but proper references
> associated with the consumers of the pll0_out clock are already taken
> below. So unless we find a better way to detect if the pll references
> are specified in the device tree or not, maybe something like this
> instead:
>

Thanks. It looks more reasonable. I will follow it in next version.

> priv->pll[0] = clk_get(priv->dev, "pll0_out);

The priv->pll[] are clk_hw* struct no clk* struct and this could be failed
when building. So maybe use a temporary clk* struct.


> if (IS_ERR(priv->pll[0])) {
> /* 24MHZ -> 1000.0MHz */
> priv->pll[0] = ...
> ...
>
> } else {
> clk_put(priv->pll[0]);
> priv->pll[0] = NULL;

>
>> + /* Use fixed factor clocks if can not get the PLL clocks from DTS */
>> + if (use_fixed_pll) {
>> /* 24MHz -> 1000.0MHz */
>
> These comments are not indented with the code, which just looks weird.

Will fix.

>
>> - priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
>> - "osc", 0, 125, 3);
>> - if (IS_ERR(priv->pll[0]))
>> - return PTR_ERR(priv->pll[0]);
>> + priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
>> + "osc", 0, 125, 3);
>> + if (IS_ERR(priv->pll[0]))
>> + return PTR_ERR(priv->pll[0]);
>>
>> /* 24MHz -> 1066.0MHz */
>> - priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
>> - "osc", 0, 533, 12);
>> - if (IS_ERR(priv->pll[1]))
>> - return PTR_ERR(priv->pll[1]);
>> + priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
>> + "osc", 0, 533, 12);
>> + if (IS_ERR(priv->pll[1]))
>> + return PTR_ERR(priv->pll[1]);
>>
>> /* 24MHz -> 1188.0MHz */
>> - priv->pll[2] = devm_clk_hw_register_fixed_factor(priv->dev, "pll2_out",
>> - "osc", 0, 99, 2);
>> - if (IS_ERR(priv->pll[2]))
>> - return PTR_ERR(priv->pll[2]);
>> + priv->pll[2] = devm_clk_hw_register_fixed_factor(priv->dev, "pll2_out",
>> + "osc", 0, 99, 2);
>> + if (IS_ERR(priv->pll[2]))
>> + return PTR_ERR(priv->pll[2]);
>> + }
>>
>> for (idx = 0; idx < JH7110_SYSCLK_END; idx++) {
>> u32 max = jh7110_sysclk_data[idx].max;
>> @@ -462,8 +465,14 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
>> parents[i].fw_name = "tdm_ext";
>> else if (pidx == JH7110_SYSCLK_MCLK_EXT)
>> parents[i].fw_name = "mclk_ext";
>> - else
>> + else if (use_fixed_pll)
>
> else if (priv->pll[0])

Will change.

>
>> parents[i].hw = priv->pll[pidx - JH7110_SYSCLK_PLL0_OUT];
>> + else if (pidx == JH7110_SYSCLK_PLL0_OUT)
>> + parents[i].fw_name = "pll0_out";
>> + else if (pidx == JH7110_SYSCLK_PLL1_OUT)
>> + parents[i].fw_name = "pll1_out";
>> + else if (pidx == JH7110_SYSCLK_PLL2_OUT)
>> + parents[i].fw_name = "pll2_out";
>> }
>>
>> clk->hw.init = &init;
>> --
>> 2.25.1
>>

Best regards,
Xingyu Wu

2023-07-14 09:56:44

by Emil Renner Berthing

Subject: Re: [RESEND PATCH v6 5/7] clk: starfive: jh7110-sys: Add PLL clocks source from DTS

On Fri, 14 Jul 2023 at 10:05, Xingyu Wu <[email protected]> wrote:
>
> On 2023/7/13 21:15, Emil Renner Berthing wrote:
> > On Tue, 4 Jul 2023 at 08:49, Xingyu Wu <[email protected]> wrote:
> >>
> >> Modify PLL clocks source to be got from DTS or
> >> the fixed factor clocks.
> >>
> >> Signed-off-by: Xingyu Wu <[email protected]>
> >> ---
> >> drivers/clk/starfive/Kconfig | 1 +
> >> .../clk/starfive/clk-starfive-jh7110-sys.c | 45 +++++++++++--------
> >> 2 files changed, 28 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
> >> index 5195f7be5213..978b78ec08b1 100644
> >> --- a/drivers/clk/starfive/Kconfig
> >> +++ b/drivers/clk/starfive/Kconfig
> >> @@ -35,6 +35,7 @@ config CLK_STARFIVE_JH7110_SYS
> >> select AUXILIARY_BUS
> >> select CLK_STARFIVE_JH71X0
> >> select RESET_STARFIVE_JH7110 if RESET_CONTROLLER
> >> + select CLK_STARFIVE_JH7110_PLL
> >> default ARCH_STARFIVE
> >> help
> >> Say yes here to support the system clock controller on the
> >> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> >> index e6031345ef05..d56f48013388 100644
> >> --- a/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> >> +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c
> >> @@ -7,6 +7,7 @@
> >> */
> >>
> >> #include <linux/auxiliary_bus.h>
> >> +#include <linux/clk.h>
> >> #include <linux/clk-provider.h>
> >> #include <linux/init.h>
> >> #include <linux/io.h>
> >> @@ -386,6 +387,7 @@ EXPORT_SYMBOL_GPL(jh7110_reset_controller_register);
> >>
> >> static int __init jh7110_syscrg_probe(struct platform_device *pdev)
> >> {
> >> + bool use_fixed_pll = true; /* PLL clocks use fixed factor clocks or PLL driver */
> >
> > nit: reverse christmas tree ordering, eg. move this below priv
> >
> >> struct jh71x0_clk_priv *priv;
> >> unsigned int idx;
> >> int ret;
> >> @@ -402,28 +404,29 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
> >> if (IS_ERR(priv->base))
> >> return PTR_ERR(priv->base);
> >>
> >> - /*
> >> - * These PLL clocks are not actually fixed factor clocks and can be
> >> - * controlled by the syscon registers of JH7110. They will be dropped
> >> - * and registered in the PLL clock driver instead.
> >> - */
> >> + if (!IS_ERR(devm_clk_get(priv->dev, "pll0_out")))
> >> + use_fixed_pll = false; /* can get pll clocks from PLL driver */
> >
> > The devm_clk_get() variant will allocate memory for a callback to call
> > clk_put() when the driver is unloaded, but proper references
> > associated with the consumers of the pll0_out clock are already taken
> > below. So unless we find a better way to detect if the pll references
> > are specified in the device tree or not, maybe something like this
> > instead:
> >
>
> Thanks. It looks more reasonable. I will follow it in next version.
>
> > priv->pll[0] = clk_get(priv->dev, "pll0_out);
>
> The priv->pll[] are clk_hw* struct no clk* struct and this could be failed
> when building. So maybe use a temporary clk* struct.

Ah yes, you're right. You'll need a local struct clk *clk for that then.

> > if (IS_ERR(priv->pll[0])) {
> > /* 24MHZ -> 1000.0MHz */
> > priv->pll[0] = ...
> > ...
> >
> > } else {
> > clk_put(priv->pll[0]);
> > priv->pll[0] = NULL;
>
> >
> >> + /* Use fixed factor clocks if can not get the PLL clocks from DTS */
> >> + if (use_fixed_pll) {
> >> /* 24MHz -> 1000.0MHz */
> >
> > These comments are not indented with the code, which just looks weird.
>
> Will fix.
>
> >
> >> - priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
> >> - "osc", 0, 125, 3);
> >> - if (IS_ERR(priv->pll[0]))
> >> - return PTR_ERR(priv->pll[0]);
> >> + priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
> >> + "osc", 0, 125, 3);
> >> + if (IS_ERR(priv->pll[0]))
> >> + return PTR_ERR(priv->pll[0]);
> >>
> >> /* 24MHz -> 1066.0MHz */
> >> - priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
> >> - "osc", 0, 533, 12);
> >> - if (IS_ERR(priv->pll[1]))
> >> - return PTR_ERR(priv->pll[1]);
> >> + priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
> >> + "osc", 0, 533, 12);
> >> + if (IS_ERR(priv->pll[1]))
> >> + return PTR_ERR(priv->pll[1]);
> >>
> >> /* 24MHz -> 1188.0MHz */
> >> - priv->pll[2] = devm_clk_hw_register_fixed_factor(priv->dev, "pll2_out",
> >> - "osc", 0, 99, 2);
> >> - if (IS_ERR(priv->pll[2]))
> >> - return PTR_ERR(priv->pll[2]);
> >> + priv->pll[2] = devm_clk_hw_register_fixed_factor(priv->dev, "pll2_out",
> >> + "osc", 0, 99, 2);
> >> + if (IS_ERR(priv->pll[2]))
> >> + return PTR_ERR(priv->pll[2]);
> >> + }
> >>
> >> for (idx = 0; idx < JH7110_SYSCLK_END; idx++) {
> >> u32 max = jh7110_sysclk_data[idx].max;
> >> @@ -462,8 +465,14 @@ static int __init jh7110_syscrg_probe(struct platform_device *pdev)
> >> parents[i].fw_name = "tdm_ext";
> >> else if (pidx == JH7110_SYSCLK_MCLK_EXT)
> >> parents[i].fw_name = "mclk_ext";
> >> - else
> >> + else if (use_fixed_pll)
> >
> > else if (priv->pll[0])
>
> Will change.
>
> >
> >> parents[i].hw = priv->pll[pidx - JH7110_SYSCLK_PLL0_OUT];
> >> + else if (pidx == JH7110_SYSCLK_PLL0_OUT)
> >> + parents[i].fw_name = "pll0_out";
> >> + else if (pidx == JH7110_SYSCLK_PLL1_OUT)
> >> + parents[i].fw_name = "pll1_out";
> >> + else if (pidx == JH7110_SYSCLK_PLL2_OUT)
> >> + parents[i].fw_name = "pll2_out";
> >> }
> >>
> >> clk->hw.init = &init;
> >> --
> >> 2.25.1
> >>
>
> Best regards,
> Xingyu Wu