* [PATCH RFC 0/3] Add Versa3 clock generator support
@ 2023-02-20 13:13 Biju Das
2023-02-20 13:13 ` [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings Biju Das
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: Biju Das @ 2023-02-20 13:13 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Biju Das, linux-clk, Geert Uytterhoeven, Kuninori Morimoto,
Lad Prabhakar, Fabrizio Castro, Mark Brown, Takashi Iwai,
alsa-devel, linux-renesas-soc
The 5P35023 is a VersaClock programmable clock generator and
it provides 6 clk outputs {diff2, diff1, se3, se2, se1 and refin}.
It has an internal OTP memory allows the user to store the configuration
in the device. After power up, the user can change the device register
settings through the I2C interface when I2C mode is selected.
This driver is for overriding OTP default values during boot based on
a full register map from DT, and also minimal support to change the
parent of a output clock.
The motivation for developing this driver is for supporting 48KHz
playback/record with audio codec on RZ/G2L SMARC EVK.
On RZ/G2L SMARC EVK, By default audio mclk is connected to
11.2896 MHz clk which is multiple of 44.1KHz.
Please see the below default OTP configuration of Dividers connected to
output clocks.
DIV3 12.2880 MHz DIFF2--> Audio clk2
DIV5 11.2896 MHz SE1 --> Audio clk1
DIV5 11.2896 MHz SE2 --> Audio mck
DIV4 12 MHz SE3 --> This clk Not used
DIV1 25 MHz DIFF1-->Ethernet clk
Ref1-> 24MHz
With this setup, we won't be able to do 48KHz playback/record on audio codec,
as mck is always connected to 11.2896MHz clk.
But by programming the i2c, we can make use of DIV4 to generate 12.2880 MHz
and make that as parent of SE2 and there by supporting 48KHz playback/record.
A block diagram with modification can be find here[1]
[1]https://paste.pics/a253ce7cdc8720c3b5eb6953b97b25ff
DIV3 12.2880 MHz DIFF2--> Audio clk2
DIV5 11.2896 MHz SE1 --> Audio clk1
DIV5 11.2896 MHz | SE2 --> Audio mck
DIV4 12.2880 MHz |
DIV2 12 MHz SE3 --> This clk Not used
DIV1 25 MHz DIFF1--> Ethernet clk
Ref1-> 24MHz
The driver can read a full register map from the DT, and will use that
register map to initialize the clk generator when the system
boots.
and later, based on sampling rate, it switches the parent of SE2 and
support both 44.1 and 48 KHz playback/record at run time.
48KHz playback
1f: f6 --> setting Div4 as clock source for se2
Read at address 0x10049C00 : 0x300B4022 --> Setting Audio clk2 in SSI
pfd2 1 1 0 24000000
pll2 1 1 0 491519897
div4_mux 1 1 0 491519897
div4 1 1 0 12287998
se2_mux 1 1 0 12287998
se2 1 1 0 12287998
44.1KHz playback
1f: b6 --> setting Div5 as clock source for se2
Read at address 0x10049C00: 0x700B4022--> Setting Audio clk1 in SSI
pfd3_mux 1 1 0 24000000
pfd3 1 1 0 960000
pll3 1 1 0 564480000
div5 1 1 0 11289600
se2_mux 1 1 0 11289600
se2 1 1 0 11289600
Please provide your valuable comment for this patch series.
Biju Das (3):
dt-bindings: clock: Add Renesas versa3 clock generator bindings
drivers: clk: Add support for versa3 clock driver
arm64: dts: renesas: rzg2l-smarc: Use versa3 clk for audio mclk
.../bindings/clock/renesas,versaclock3.yaml | 135 ++
.../boot/dts/renesas/rz-smarc-common.dtsi | 7 -
arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi | 35 +
drivers/clk/Kconfig | 9 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-versaclock3.c | 1134 +++++++++++++++++
6 files changed, 1314 insertions(+), 7 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
create mode 100644 drivers/clk/clk-versaclock3.c
--
2.25.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-02-20 13:13 [PATCH RFC 0/3] Add Versa3 clock generator support Biju Das
@ 2023-02-20 13:13 ` Biju Das
2023-02-22 9:34 ` Krzysztof Kozlowski
2023-02-20 13:13 ` [PATCH RFC 2/3] drivers: clk: Add support for versa3 clock driver Biju Das
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: Biju Das @ 2023-02-20 13:13 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Rob Herring, Krzysztof Kozlowski
Cc: Biju Das, Geert Uytterhoeven, linux-renesas-soc, linux-clk,
devicetree, Fabrizio Castro
Document Renesas versa3 clock generator(5P35023) bindings.
The 5P35023 is a VersaClock programmable clock generator and
is designed for low-power, consumer, and high-performance PCI
Express applications. The 5P35023 device is a three PLL
architecture design, and each PLL is individually programmable
and allowing for up to 6 unique frequency outputs.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
.../bindings/clock/renesas,versaclock3.yaml | 135 ++++++++++++++++++
1 file changed, 135 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
diff --git a/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
new file mode 100644
index 000000000000..f45b8da73ec3
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
@@ -0,0 +1,135 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/renesas,versaclock3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas VersaClock 3 programmable I2C clock generators
+
+description: |
+ The 5P35023 is a VersaClock programmable clock generator and
+ is designed for low-power, consumer, and high-performance PCI
+ express applications. The 5P35023 device is a three PLL
+ architecture design, and each PLL is individually programmable
+ and allowing for up to 6 unique frequency outputs.
+
+ An internal OTP memory allows the user to store the configuration
+ in the device. After power up, the user can change the device register
+ settings through the I2C interface when I2C mode is selected.
+
+ The driver can read a full register map from the DT, and will use that
+ register map to initialize the attached part (via I2C) when the system
+ boots. Any configuration not supported by the common clock framework
+ must be done via the full register map, including optimized settings.
+
+ Link to datasheet: https://www.renesas.com/us/en/products/clocks-timing/
+ clock-generation/programmable-clocks/
+ 5p35023-versaclock-3s-programmable-clock-generator
+
+maintainers:
+ - Biju Das <biju.das.jz@bp.renesas.com>
+
+properties:
+ compatible:
+ enum:
+ - renesas,5p35023
+
+ reg:
+ description: I2C device address
+ maxItems: 1
+
+ '#clock-cells':
+ const: 1
+
+ clock-names:
+ oneOf:
+ - items:
+ - const: x1
+ - items:
+ - const: clkin
+
+ clocks:
+ maxItems: 1
+
+ renesas,settings:
+ description: Optional, complete register map of the device.
+ Optimized settings for the device must be provided in full
+ and are written during initialization.
+ $ref: /schemas/types.yaml#/definitions/uint8-array
+ minItems: 37
+
+ assigned-clocks:
+ minItems: 6
+
+ assigned-clock-rates:
+ minItems: 6
+
+ renesas,clock-divider-read-only:
+ description: Flag for setting divider in read only mode.
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 5
+
+ renesas,clock-flags:
+ description: Flags used in common clock frame work for configuring
+ clk outputs. See include/linux/clk-provider.h
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 6
+
+required:
+ - compatible
+ - reg
+ - '#clock-cells'
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+ /* 24MHz crystal */
+ x1_x2: xtal {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <24000000>;
+ };
+
+ i2c@0 {
+ reg = <0x0 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ versa3: clock-generator@68 {
+ compatible = "renesas,5p35023";
+ reg = <0x68>;
+ #clock-cells = <1>;
+
+ clocks = <&x1_x2>;
+ clock-names = "x1";
+
+ renesas,settings = [
+ 80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
+ 00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
+ 80 b0 45 c4 95
+ ];
+
+ assigned-clocks = <&versa3 0>,
+ <&versa3 1>,
+ <&versa3 2>,
+ <&versa3 3>,
+ <&versa3 4>,
+ <&versa3 5>;
+ assigned-clock-rates = <12288000>, <25000000>,
+ <12000000>, <11289600>,
+ <11289600>, <24000000>;
+ renesas,clock-divider-read-only = <1>, <1>, <1>, <1>, <1>;
+ renesas,clock-flags = <2176>, <2176>, <2176>, <2052>,
+ <2176>, <2048>;
+ };
+ };
+
+ /* Consumer referencing the versa 3 */
+ consumer {
+ /* ... */
+ clocks = <&versa3 3>;
+ /* ... */
+ };
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH RFC 2/3] drivers: clk: Add support for versa3 clock driver
2023-02-20 13:13 [PATCH RFC 0/3] Add Versa3 clock generator support Biju Das
2023-02-20 13:13 ` [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings Biju Das
@ 2023-02-20 13:13 ` Biju Das
2023-03-06 20:22 ` Stephen Boyd
2023-02-20 13:13 ` [PATCH RFC 3/3] arm64: dts: renesas: rzg2l-smarc: Use versa3 clk for audio mclk Biju Das
2023-02-20 13:18 ` [PATCH RFC 0/3] Add Versa3 clock generator support Biju Das
3 siblings, 1 reply; 26+ messages in thread
From: Biju Das @ 2023-02-20 13:13 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Biju Das, linux-clk, Geert Uytterhoeven, Fabrizio Castro,
linux-renesas-soc
Add support for Renesas versa3 clock driver(5p35023).
The clock generator provides 6 output clocks.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
drivers/clk/Kconfig | 9 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-versaclock3.c | 1134 +++++++++++++++++++++++++++++++++
3 files changed, 1144 insertions(+)
create mode 100644 drivers/clk/clk-versaclock3.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index b6c5bf69a2b2..4dfa44cf5289 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -367,6 +367,15 @@ config COMMON_CLK_RS9_PCIE
This driver supports the Renesas 9-series PCIe clock generator
models 9FGV/9DBV/9DMV/9FGL/9DML/9QXL/9SQ.
+config COMMON_CLK_VC3
+ tristate "Clock driver for Renesas VersaClock 3 devices"
+ depends on I2C
+ depends on OF
+ select REGMAP_I2C
+ help
+ This driver supports the Renesas VersaClock 3 programmable clock
+ generators.
+
config COMMON_CLK_VC5
tristate "Clock driver for IDT VersaClock 5,6 devices"
depends on I2C
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index e3ca0d058a25..e7aa4f81863c 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -72,6 +72,7 @@ obj-$(CONFIG_COMMON_CLK_TPS68470) += clk-tps68470.o
obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o
obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
obj-$(CONFIG_COMMON_CLK_RS9_PCIE) += clk-renesas-pcie.o
+obj-$(CONFIG_COMMON_CLK_VC3) += clk-versaclock3.o
obj-$(CONFIG_COMMON_CLK_VC5) += clk-versaclock5.o
obj-$(CONFIG_COMMON_CLK_VC7) += clk-versaclock7.o
obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c
new file mode 100644
index 000000000000..561cfad8a243
--- /dev/null
+++ b/drivers/clk/clk-versaclock3.c
@@ -0,0 +1,1134 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for Renesas Versaclock 3
+ *
+ * Copyright (C) 2021 Renesas Electronics Corp.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/i2c.h>
+#include <linux/limits.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define NUM_CONFIG_REGISTERS 37
+
+#define VC3_GENERAL_CTR 0x0
+#define VC3_GENERAL_CTR_DIV1_SRC_SEL BIT(3)
+#define VC3_GENERAL_CTR_PLL3_REFIN_SEL BIT(2)
+
+#define VC3_PLL3_M_DIVIDER 0x3
+#define VC3_PLL3_M_DIV1 BIT(7)
+#define VC3_PLL3_M_DIV2 BIT(6)
+#define VC3_PLL3_M_DIV(n) ((n) & GENMASK(5, 0))
+
+#define VC3_PLL3_N_DIVIDER 0x4
+#define VC3_PLL3_LOOP_FILTER_N_DIV_MSB 0x5
+
+#define VC3_PLL3_CHARGE_PUMP_CTRL 0x6
+#define VC3_PLL3_CHARGE_PUMP_CTRL_OUTDIV3_SRC_SEL BIT(7)
+
+#define VC3_PLL1_CTRL_OUTDIV5 0x7
+#define VC3_PLL1_CTRL_OUTDIV5_PLL1_MDIV_DOUBLER BIT(7)
+
+#define VC3_PLL1_M_DIVIDER 0x8
+#define VC3_PLL1_M_DIV1 BIT(7)
+#define VC3_PLL1_M_DIV2 BIT(6)
+#define VC3_PLL1_M_DIV(n) ((n) & GENMASK(5, 0))
+
+#define VC3_PLL1_VCO_N_DIVIDER 0x9
+#define VC3_PLL1_LOOP_FILTER_N_DIV_MSB 0x0a
+
+#define VC3_OUT_DIV1_DIV2_CTRL 0xf
+
+#define VC3_PLL2_FB_INT_DIV_MSB 0x10
+#define VC3_PLL2_FB_INT_DIV_LSB 0x11
+#define VC3_PLL2_FB_FRC_DIV_MSB 0x12
+#define VC3_PLL2_FB_FRC_DIV_LSB 0x13
+
+#define VC3_PLL2_M_DIVIDER 0x1a
+#define VC3_PLL2_MDIV_DOUBLER BIT(7)
+#define VC3_PLL2_M_DIV1 BIT(6)
+#define VC3_PLL2_M_DIV2 BIT(5)
+#define VC3_PLL2_M_DIV(n) ((n) & GENMASK(4, 0))
+
+#define VC3_OUT_DIV3_DIV4_CTRL 0x1b
+
+#define VC3_PLL_OP_CTRL 0x1c
+#define VC3_PLL_OP_CTRL_PLL2_REFIN_SEL 6
+
+#define VC3_OUTPUT_CTR 0x1d
+#define VC3_OUTPUT_CTR_DIV4_SRC_SEL BIT(3)
+
+#define VC3_SE2_CTRL_REG0 0x1f
+#define VC3_SE2_CTRL_REG0_SE2_CLK_SEL BIT(6)
+
+#define VC3_SE3_DIFF1_CTRL_REG 0x21
+#define VC3_SE3_DIFF1_CTRL_REG_SE3_CLK_SEL BIT(6)
+
+#define VC3_DIFF1_CTRL_REG 0x22
+#define VC3_DIFF1_CTRL_REG_DIFF1_CLK_SEL BIT(7)
+
+#define VC3_DIFF2_CTRL_REG 0x23
+#define VC3_DIFF2_CTRL_REG_DIFF2_CLK_SEL BIT(7)
+
+#define VC3_SE1_DIV4_CTRL 0x24
+#define VC3_SE1_DIV4_CTRL_SE1_CLK_SEL BIT(3)
+
+#define VC3_PLL1_VCO_MIN 300000000UL
+#define VC3_PLL1_VCO_MAX 600000000UL
+
+#define VC3_PLL2_VCO_MIN 400000000UL
+#define VC3_PLL2_VCO_MAX 1200000000UL
+
+#define VC3_PLL3_VCO_MIN 300000000UL
+#define VC3_PLL3_VCO_MAX 800000000UL
+
+#define VC3_2_POW_16 (U16_MAX + 1)
+#define VC3_DIV_MASK(width) ((1 << (width)) - 1)
+
+struct vc3_driver_data;
+
+enum vc3_pfd_mux {
+ VC3_PFD2_MUX,
+ VC3_PFD3_MUX,
+};
+
+enum vc3_pfd {
+ VC3_PFD1,
+ VC3_PFD2,
+ VC3_PFD3,
+};
+
+enum vc3_pll {
+ VC3_PLL1,
+ VC3_PLL2,
+ VC3_PLL3,
+};
+
+enum vc3_div_mux {
+ VC3_DIV1_MUX,
+ VC3_DIV3_MUX,
+ VC3_DIV4_MUX,
+};
+
+enum vc3_div {
+ VC3_DIV1,
+ VC3_DIV2,
+ VC3_DIV3,
+ VC3_DIV4,
+ VC3_DIV5,
+};
+
+enum vc3_clk_mux {
+ VC3_DIFF2_MUX,
+ VC3_DIFF1_MUX,
+ VC3_SE3_MUX,
+ VC3_SE2_MUX,
+ VC3_SE1_MUX,
+};
+
+enum vc3_clk {
+ VC3_DIFF2,
+ VC3_DIFF1,
+ VC3_SE3,
+ VC3_SE2,
+ VC3_SE1,
+ VC3_REF,
+};
+
+struct vc3_clk_data {
+ char *name;
+ u8 offs;
+ u8 bitmsk;
+ const char *parent_names[2];
+};
+
+struct vc3_pfd_data {
+ char *name;
+ u8 offs;
+ u8 mdiv1_bitmsk;
+ u8 mdiv2_bitmsk;
+};
+
+struct vc3_pll_data {
+ char *name;
+ u8 int_div_msb_offs;
+ u8 int_div_lsb_offs;
+ unsigned long vco_min;
+ unsigned long vco_max;
+};
+
+struct vc3_div_data {
+ char *name;
+ u8 offs;
+ const struct clk_div_table *table;
+ u8 shift;
+ u8 width;
+};
+
+static const struct clk_div_table div1_divs[] = {
+ { .val = 0, .div = 1, }, { .val = 1, .div = 4, },
+ { .val = 2, .div = 5, }, { .val = 3, .div = 6, },
+ { .val = 4, .div = 2, }, { .val = 5, .div = 8, },
+ { .val = 6, .div = 10, }, { .val = 7, .div = 12, },
+ { .val = 8, .div = 4, }, { .val = 9, .div = 16, },
+ { .val = 10, .div = 20, }, { .val = 11, .div = 24, },
+ { .val = 12, .div = 8, }, { .val = 13, .div = 32, },
+ { .val = 14, .div = 40, }, { .val = 15, .div = 48, },
+ {}
+};
+
+static const struct clk_div_table div245_divs[] = {
+ { .val = 0, .div = 1, }, { .val = 1, .div = 3, },
+ { .val = 2, .div = 5, }, { .val = 3, .div = 10, },
+ { .val = 4, .div = 2, }, { .val = 5, .div = 6, },
+ { .val = 6, .div = 10, }, { .val = 7, .div = 20, },
+ { .val = 8, .div = 4, }, { .val = 9, .div = 12, },
+ { .val = 10, .div = 20, }, { .val = 11, .div = 40, },
+ { .val = 12, .div = 5, }, { .val = 13, .div = 15, },
+ { .val = 14, .div = 25, }, { .val = 15, .div = 50, },
+ {}
+};
+
+static const struct clk_div_table div3_divs[] = {
+ { .val = 0, .div = 1, }, { .val = 1, .div = 3, },
+ { .val = 2, .div = 5, }, { .val = 3, .div = 10, },
+ { .val = 4, .div = 2, }, { .val = 5, .div = 6, },
+ { .val = 6, .div = 10, }, { .val = 7, .div = 20, },
+ { .val = 8, .div = 4, }, { .val = 9, .div = 12, },
+ { .val = 10, .div = 20, }, { .val = 11, .div = 40, },
+ { .val = 12, .div = 8, }, { .val = 13, .div = 24, },
+ { .val = 14, .div = 40, }, { .val = 15, .div = 80, },
+ {}
+};
+
+static const struct vc3_div_data div_data[] = {
+ [VC3_DIV1] = {
+ .name = "div1",
+ .offs = VC3_OUT_DIV1_DIV2_CTRL,
+ .table = div1_divs,
+ .shift = 4,
+ .width = 4,
+ },
+ [VC3_DIV2] = {
+ .name = "div2",
+ .offs = VC3_OUT_DIV1_DIV2_CTRL,
+ .table = div245_divs,
+ .shift = 0,
+ .width = 4,
+ },
+ [VC3_DIV3] = {
+ .name = "div3",
+ .offs = VC3_OUT_DIV3_DIV4_CTRL,
+ .table = div3_divs,
+ .shift = 4,
+ .width = 4,
+ },
+ [VC3_DIV4] = {
+ .name = "div4",
+ .offs = VC3_OUT_DIV3_DIV4_CTRL,
+ .table = div245_divs,
+ .shift = 0,
+ .width = 4,
+ },
+ [VC3_DIV5] = {
+ .name = "div5",
+ .offs = VC3_PLL1_CTRL_OUTDIV5,
+ .table = div245_divs,
+ .shift = 0,
+ .width = 4,
+ },
+};
+
+static const struct vc3_clk_data pfd_mux_data[] = {
+ [VC3_PFD2_MUX] = {
+ .name = "pfd2_mux",
+ .offs = VC3_PLL_OP_CTRL,
+ .bitmsk = BIT(VC3_PLL_OP_CTRL_PLL2_REFIN_SEL),
+ .parent_names[1] = div_data[VC3_DIV2].name,
+ },
+ [VC3_PFD3_MUX] = {
+ .name = "pfd3_mux",
+ .offs = VC3_GENERAL_CTR,
+ .bitmsk = BIT(VC3_GENERAL_CTR_PLL3_REFIN_SEL),
+ .parent_names[1] = div_data[VC3_DIV2].name,
+ },
+};
+
+static const struct vc3_pfd_data pfd_data[] = {
+ [VC3_PFD1] = {
+ .name = "pfd1",
+ .offs = VC3_PLL1_M_DIVIDER,
+ .mdiv1_bitmsk = VC3_PLL1_M_DIV1,
+ .mdiv2_bitmsk = VC3_PLL1_M_DIV2
+ },
+ [VC3_PFD2] = {
+ .name = "pfd2",
+ .offs = VC3_PLL2_M_DIVIDER,
+ .mdiv1_bitmsk = VC3_PLL2_M_DIV1,
+ .mdiv2_bitmsk = VC3_PLL2_M_DIV2
+ },
+ [VC3_PFD3] = {
+ .name = "pfd3",
+ .offs = VC3_PLL3_M_DIVIDER,
+ .mdiv1_bitmsk = VC3_PLL3_M_DIV1,
+ .mdiv2_bitmsk = VC3_PLL3_M_DIV2
+ },
+};
+
+static const struct vc3_pll_data pll_data[] = {
+ [VC3_PLL1] = {
+ .name = "pll1",
+ .int_div_msb_offs = VC3_PLL1_LOOP_FILTER_N_DIV_MSB,
+ .int_div_lsb_offs = VC3_PLL1_VCO_N_DIVIDER,
+ .vco_min = VC3_PLL1_VCO_MIN,
+ .vco_max = VC3_PLL1_VCO_MAX,
+ },
+ [VC3_PLL2] = {
+ .name = "pll2",
+ .int_div_msb_offs = VC3_PLL2_FB_INT_DIV_MSB,
+ .int_div_lsb_offs = VC3_PLL2_FB_INT_DIV_LSB,
+ .vco_min = VC3_PLL2_VCO_MIN,
+ .vco_max = VC3_PLL2_VCO_MAX,
+ },
+ [VC3_PLL3] = {
+ .name = "pll3",
+ .int_div_msb_offs = VC3_PLL3_LOOP_FILTER_N_DIV_MSB,
+ .int_div_lsb_offs = VC3_PLL3_N_DIVIDER,
+ .vco_min = VC3_PLL3_VCO_MIN,
+ .vco_max = VC3_PLL3_VCO_MAX,
+ },
+};
+
+static const struct vc3_clk_data div_mux_data[] = {
+ [VC3_DIV1_MUX] = {
+ .name = "div1_mux",
+ .offs = VC3_GENERAL_CTR,
+ .bitmsk = VC3_GENERAL_CTR_DIV1_SRC_SEL,
+ .parent_names[0] = pll_data[VC3_PLL1].name,
+ },
+ [VC3_DIV3_MUX] = {
+ .name = "div3_mux",
+ .offs = VC3_PLL3_CHARGE_PUMP_CTRL,
+ .bitmsk = VC3_PLL3_CHARGE_PUMP_CTRL_OUTDIV3_SRC_SEL,
+ .parent_names[0] = pll_data[VC3_PLL2].name,
+ .parent_names[1] = pll_data[VC3_PLL3].name,
+ },
+ [VC3_DIV4_MUX] = {
+ .name = "div4_mux",
+ .offs = VC3_OUTPUT_CTR,
+ .bitmsk = VC3_OUTPUT_CTR_DIV4_SRC_SEL,
+ .parent_names[0] = pll_data[VC3_PLL2].name,
+ },
+};
+
+static const struct vc3_clk_data clk_mux_data[] = {
+ [VC3_DIFF2_MUX] = {
+ .name = "diff2_mux",
+ .offs = VC3_DIFF2_CTRL_REG,
+ .bitmsk = VC3_DIFF2_CTRL_REG_DIFF2_CLK_SEL,
+ .parent_names[0] = div_data[VC3_DIV1].name,
+ .parent_names[1] = div_data[VC3_DIV3].name,
+ },
+ [VC3_DIFF1_MUX] = {
+ .name = "diff1_mux",
+ .offs = VC3_DIFF1_CTRL_REG,
+ .bitmsk = VC3_DIFF1_CTRL_REG_DIFF1_CLK_SEL,
+ .parent_names[0] = div_data[VC3_DIV1].name,
+ .parent_names[1] = div_data[VC3_DIV3].name,
+ },
+ [VC3_SE3_MUX] = {
+ .name = "se3_mux",
+ .offs = VC3_SE3_DIFF1_CTRL_REG,
+ .bitmsk = VC3_SE3_DIFF1_CTRL_REG_SE3_CLK_SEL,
+ .parent_names[0] = div_data[VC3_DIV2].name,
+ .parent_names[1] = div_data[VC3_DIV4].name,
+ },
+ [VC3_SE2_MUX] = {
+ .name = "se2_mux",
+ .offs = VC3_SE2_CTRL_REG0,
+ .bitmsk = VC3_SE2_CTRL_REG0_SE2_CLK_SEL,
+ .parent_names[0] = div_data[VC3_DIV5].name,
+ .parent_names[1] = div_data[VC3_DIV4].name,
+ },
+ [VC3_SE1_MUX] = {
+ .name = "se1_mux",
+ .offs = VC3_SE1_DIV4_CTRL,
+ .bitmsk = VC3_SE1_DIV4_CTRL_SE1_CLK_SEL,
+ .parent_names[0] = div_data[VC3_DIV5].name,
+ .parent_names[1] = div_data[VC3_DIV4].name,
+ },
+};
+
+static const struct vc3_clk_data clk_out_data[] = {
+ [VC3_DIFF2] = {
+ .name = "diff2",
+ .parent_names[0] = clk_mux_data[VC3_DIFF2_MUX].name,
+ },
+ [VC3_DIFF1] = {
+ .name = "diff1",
+ .parent_names[0] = clk_mux_data[VC3_DIFF1_MUX].name,
+ },
+ [VC3_SE3] = {
+ .name = "se3",
+ .parent_names[0] = clk_mux_data[VC3_SE3_MUX].name,
+ },
+ [VC3_SE2] = {
+ .name = "se2",
+ .parent_names[0] = clk_mux_data[VC3_SE2_MUX].name,
+ },
+ [VC3_SE1] = {
+ .name = "se1",
+ .parent_names[0] = clk_mux_data[VC3_SE1_MUX].name,
+ },
+ [VC3_REF] = {
+ .name = "ref",
+ },
+};
+
+struct vc3_hw_data {
+ struct clk_hw hw;
+ struct vc3_driver_data *vc3;
+ const void *data;
+ unsigned int num;
+ u32 div_int;
+ u32 div_frc;
+ u8 flags;
+};
+
+struct vc3_driver_data {
+ struct i2c_client *client;
+ struct regmap *regmap;
+
+ struct vc3_hw_data clk_pfd_mux[ARRAY_SIZE(pfd_mux_data)];
+ struct vc3_hw_data clk_pfd[ARRAY_SIZE(pfd_data)];
+ struct vc3_hw_data clk_pll[ARRAY_SIZE(pll_data)];
+ struct vc3_hw_data clk_div_mux[ARRAY_SIZE(div_mux_data)];
+ struct vc3_hw_data clk_div[ARRAY_SIZE(div_data)];
+ struct vc3_hw_data clk_mux[ARRAY_SIZE(clk_mux_data)];
+ struct vc3_hw_data clk_out[ARRAY_SIZE(clk_out_data)];
+};
+
+static unsigned char vc3_pfd_mux_get_parent(struct clk_hw *hw)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_clk_data *pfd_mux = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+ u32 src;
+
+ regmap_read(vc3->regmap, pfd_mux->offs, &src);
+
+ return !!(src & pfd_mux->bitmsk);
+}
+
+static int vc3_pfd_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_clk_data *pfd_mux = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+
+ regmap_update_bits(vc3->regmap, pfd_mux->offs, pfd_mux->bitmsk,
+ index ? pfd_mux->bitmsk : 0);
+ return 0;
+}
+
+static const struct clk_ops vc3_pfd_mux_ops = {
+ .set_parent = vc3_pfd_mux_set_parent,
+ .get_parent = vc3_pfd_mux_get_parent,
+};
+
+static unsigned long vc3_pfd_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_pfd_data *pfd = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+ unsigned int prediv, premul;
+ unsigned long rate;
+ u8 mdiv;
+
+ regmap_read(vc3->regmap, pfd->offs, &prediv);
+ if (hwdata->num == VC3_PFD1) {
+ /* The bypass_prediv is set, PLL fed from Ref_in directly. */
+ if (prediv & pfd->mdiv1_bitmsk) {
+ /* check doubler is set or not */
+ regmap_read(vc3->regmap, VC3_PLL1_CTRL_OUTDIV5, &premul);
+ if (premul & VC3_PLL1_CTRL_OUTDIV5_PLL1_MDIV_DOUBLER)
+ parent_rate *= 2;
+ return parent_rate;
+ }
+ mdiv = VC3_PLL1_M_DIV(prediv);
+ } else if (hwdata->num == VC3_PFD2) {
+ /* The bypass_prediv is set, PLL fed from Ref_in directly. */
+ if (prediv & pfd->mdiv1_bitmsk) {
+ /* check doubler is set or not */
+ if (premul & VC3_PLL2_MDIV_DOUBLER)
+ parent_rate *= 2;
+ return parent_rate;
+ }
+
+ mdiv = VC3_PLL2_M_DIV(prediv);
+ } else {
+ /* The bypass_prediv is set, PLL fed from Ref_in directly. */
+ if (prediv & pfd->mdiv1_bitmsk)
+ return parent_rate;
+
+ mdiv = VC3_PLL3_M_DIV(prediv);
+ }
+
+ if (prediv & pfd->mdiv2_bitmsk)
+ rate = parent_rate / 2;
+ else
+ rate = parent_rate / mdiv;
+
+ return rate;
+}
+
+static long vc3_pfd_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ unsigned long idiv;
+
+ /* PLL cannot operate with input clock above 50 MHz. */
+ if (rate > 50000000)
+ return -EINVAL;
+
+ /* CLKIN within range of PLL input, feed directly to PLL. */
+ if (*parent_rate <= 50000000)
+ return *parent_rate;
+
+ idiv = DIV_ROUND_UP(*parent_rate, rate);
+ if (hwdata->num == VC3_PFD1 || hwdata->num == VC3_PFD3) {
+ if (idiv > 63)
+ return -EINVAL;
+ } else {
+ if (idiv > 31)
+ return -EINVAL;
+ }
+
+ return *parent_rate / idiv;
+}
+
+static int vc3_pfd_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_pfd_data *pfd = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+ unsigned long idiv;
+ u8 div;
+
+ /* CLKIN within range of PLL input, feed directly to PLL. */
+ if (parent_rate <= 50000000) {
+ regmap_update_bits(vc3->regmap, pfd->offs, pfd->mdiv1_bitmsk,
+ pfd->mdiv1_bitmsk);
+ regmap_update_bits(vc3->regmap, pfd->offs, pfd->mdiv2_bitmsk, 0);
+ return 0;
+ }
+
+ idiv = DIV_ROUND_UP(parent_rate, rate);
+ /* We have dedicated div-2 predivider. */
+ if (idiv == 2) {
+ regmap_update_bits(vc3->regmap, pfd->offs, pfd->mdiv2_bitmsk,
+ pfd->mdiv2_bitmsk);
+ regmap_update_bits(vc3->regmap, pfd->offs, pfd->mdiv1_bitmsk, 0);
+ } else {
+ if (hwdata->num == VC3_PFD1)
+ div = VC3_PLL1_M_DIV(idiv);
+ else if (hwdata->num == VC3_PFD2)
+ div = VC3_PLL2_M_DIV(idiv);
+ else
+ div = VC3_PLL3_M_DIV(idiv);
+
+ regmap_write(vc3->regmap, pfd->offs, div);
+ }
+
+ return 0;
+}
+
+static const struct clk_ops vc3_pfd_ops = {
+ .recalc_rate = vc3_pfd_recalc_rate,
+ .round_rate = vc3_pfd_round_rate,
+ .set_rate = vc3_pfd_set_rate,
+};
+
+static unsigned long vc3_pll_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_pll_data *pll = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+ u32 div_int, div_frc, val;
+ unsigned long rate;
+
+ regmap_read(vc3->regmap, pll->int_div_msb_offs, &val);
+ div_int = (val & GENMASK(2, 0)) << 8;
+ regmap_read(vc3->regmap, pll->int_div_lsb_offs, &val);
+ div_int |= val;
+
+ if (hwdata->num == VC3_PLL2) {
+ regmap_read(vc3->regmap, VC3_PLL2_FB_FRC_DIV_MSB, &val);
+ div_frc = val << 8;
+ regmap_read(vc3->regmap, VC3_PLL2_FB_FRC_DIV_LSB, &val);
+ div_frc |= val;
+ rate = (parent_rate *
+ (div_int * VC3_2_POW_16 + div_frc) / VC3_2_POW_16);
+ } else {
+ rate = parent_rate * div_int;
+ }
+
+ return rate;
+}
+
+static long vc3_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_pll_data *pll = hwdata->data;
+ u64 div_frc;
+
+ if (rate < pll->vco_min)
+ rate = pll->vco_min;
+ if (rate > pll->vco_max)
+ rate = pll->vco_max;
+
+ hwdata->div_int = rate / *parent_rate;
+
+ if (hwdata->num == VC3_PLL2) {
+ if (hwdata->div_int > 0x7ff)
+ rate = *parent_rate * 0x7ff;
+
+ /* Determine best fractional part, which is 16 bit wide */
+ div_frc = rate % *parent_rate;
+ div_frc *= BIT(16) - 1;
+ do_div(div_frc, *parent_rate);
+
+ hwdata->div_frc = (u32)div_frc;
+ rate = (*parent_rate *
+ (hwdata->div_int * VC3_2_POW_16 + div_frc) / VC3_2_POW_16);
+ } else {
+ rate = *parent_rate * hwdata->div_int;
+ }
+
+ return rate;
+}
+
+static int vc3_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_pll_data *pll = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+ u32 val;
+
+ regmap_read(vc3->regmap, pll->int_div_msb_offs, &val);
+ val = (val & 0xf8) | ((hwdata->div_int >> 8) & 0x7);
+ regmap_write(vc3->regmap, pll->int_div_msb_offs, val);
+ regmap_write(vc3->regmap, pll->int_div_lsb_offs, hwdata->div_int & 0xff);
+
+ if (hwdata->num == VC3_PLL2) {
+ regmap_write(vc3->regmap, VC3_PLL2_FB_FRC_DIV_MSB,
+ hwdata->div_frc >> 8);
+ regmap_write(vc3->regmap, VC3_PLL2_FB_FRC_DIV_LSB,
+ hwdata->div_frc & 0xff);
+ }
+
+ return 0;
+}
+
+static const struct clk_ops vc3_pll_ops = {
+ .recalc_rate = vc3_pll_recalc_rate,
+ .round_rate = vc3_pll_round_rate,
+ .set_rate = vc3_pll_set_rate,
+};
+
+static unsigned char vc3_div_mux_get_parent(struct clk_hw *hw)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_clk_data *div_mux = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+ u32 src;
+
+ regmap_read(vc3->regmap, div_mux->offs, &src);
+
+ return !!(src & div_mux->bitmsk);
+}
+
+static int vc3_div_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_clk_data *div_mux = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+
+ regmap_update_bits(vc3->regmap, div_mux->offs, div_mux->bitmsk,
+ index ? div_mux->bitmsk : 0);
+
+ return 0;
+}
+
+static const struct clk_ops vc3_div_mux_ops = {
+ .set_parent = vc3_div_mux_set_parent,
+ .get_parent = vc3_div_mux_get_parent,
+};
+
+static unsigned int vc3_get_div(const struct clk_div_table *table,
+ unsigned int val, unsigned long flag)
+{
+ const struct clk_div_table *clkt;
+
+ for (clkt = table; clkt->div; clkt++)
+ if (clkt->val == val)
+ return clkt->div;
+
+ return 0;
+}
+
+static unsigned long vc3_div_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_div_data *div_data = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+ unsigned int val;
+
+ regmap_read(vc3->regmap, div_data->offs, &val);
+ val >>= div_data->shift;
+ val &= VC3_DIV_MASK(div_data->width);
+
+ return divider_recalc_rate(hw, parent_rate, val, div_data->table,
+ hwdata->flags, div_data->width);
+}
+
+static long vc3_div_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_div_data *div_data = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+ unsigned int bestdiv;
+
+ /* if read only, just return current value */
+ if (hwdata->flags & CLK_DIVIDER_READ_ONLY) {
+ regmap_read(vc3->regmap, div_data->offs, &bestdiv);
+ bestdiv >>= div_data->shift;
+ bestdiv &= VC3_DIV_MASK(div_data->width);
+ bestdiv = vc3_get_div(div_data->table, bestdiv, hwdata->flags);
+ return DIV_ROUND_UP(*parent_rate, bestdiv);
+ }
+
+ return divider_round_rate(hw, rate, parent_rate, div_data->table,
+ div_data->width, hwdata->flags);
+}
+
+static int vc3_div_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_div_data *div_data = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+ unsigned int value;
+
+ value = divider_get_val(rate, parent_rate, div_data->table,
+ div_data->width, hwdata->flags);
+ regmap_update_bits(vc3->regmap, div_data->offs,
+ VC3_DIV_MASK(div_data->width) << div_data->shift,
+ value << div_data->shift);
+ return 0;
+}
+
+static const struct clk_ops vc3_div_ops = {
+ .recalc_rate = vc3_div_recalc_rate,
+ .round_rate = vc3_div_round_rate,
+ .set_rate = vc3_div_set_rate,
+};
+
+static int vc3_clk_mux_determine_rate(struct clk_hw *hw,
+ struct clk_rate_request *req)
+{
+ int ret;
+ int frc;
+
+ ret = clk_mux_determine_rate_flags(hw, req, CLK_SET_RATE_PARENT);
+ if (ret) {
+ if (req->best_parent_rate / req->rate) {
+ frc = DIV_ROUND_CLOSEST_ULL(req->best_parent_rate,
+ req->rate);
+ req->rate *= frc;
+ return clk_mux_determine_rate_flags(hw, req,
+ CLK_SET_RATE_PARENT);
+ }
+ ret = 0;
+ }
+
+ return ret;
+}
+
+static unsigned char vc3_clk_mux_get_parent(struct clk_hw *hw)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_clk_data *clk_mux = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+ u32 val;
+
+ regmap_read(vc3->regmap, clk_mux->offs, &val);
+
+ return !!(val & clk_mux->bitmsk);
+}
+
+static int vc3_clk_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+ struct vc3_hw_data *hwdata = container_of(hw, struct vc3_hw_data, hw);
+ const struct vc3_clk_data *clk_mux = hwdata->data;
+ struct vc3_driver_data *vc3 = hwdata->vc3;
+
+ regmap_update_bits(vc3->regmap, clk_mux->offs,
+ clk_mux->bitmsk, index ? clk_mux->bitmsk : 0);
+ return 0;
+}
+
+static const struct clk_ops vc3_clk_mux_ops = {
+ .determine_rate = vc3_clk_mux_determine_rate,
+ .set_parent = vc3_clk_mux_set_parent,
+ .get_parent = vc3_clk_mux_get_parent,
+};
+
+static unsigned long vc3_clk_out_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ return parent_rate;
+}
+
+static long vc3_clk_out_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *prate)
+{
+ *prate = clk_hw_round_rate(clk_hw_get_parent(hw), rate);
+
+ return *prate;
+}
+
+static int vc3_clk_out_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ /*
+ * We must report success. round_rate() propagates rate to the
+ * parent and based on the rate mux changes its parent.
+ */
+
+ return 0;
+}
+
+const struct clk_ops vc3_clk_out_ops = {
+ .recalc_rate = vc3_clk_out_recalc_rate,
+ .round_rate = vc3_clk_out_round_rate,
+ .set_rate = vc3_clk_out_set_rate,
+};
+
+static bool vc3_regmap_is_writeable(struct device *dev, unsigned int reg)
+{
+ return true;
+}
+
+static const struct regmap_config vc3_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .cache_type = REGCACHE_RBTREE,
+ .max_register = 0x24,
+ .writeable_reg = vc3_regmap_is_writeable,
+};
+
+static struct clk_hw *vc3_of_clk_get(struct of_phandle_args *clkspec,
+ void *data)
+{
+ struct vc3_driver_data *vc3 = data;
+ unsigned int idx = clkspec->args[0];
+
+ if (idx >= ARRAY_SIZE(clk_out_data)) {
+ pr_err("invalid clk index %u for provider %pOF\n", idx, clkspec->np);
+ return ERR_PTR(-EINVAL);
+ }
+
+ return &vc3->clk_out[idx].hw;
+}
+
+static void vc3_divider_type_parse_dt(struct device *dev,
+ struct vc3_driver_data *vc3)
+{
+ struct device_node *np = dev->of_node;
+ struct property *prop;
+ const __be32 *p;
+ u32 i = 0;
+ u32 val;
+
+ of_property_for_each_u32(np, "renesas,clock-divider-read-only",
+ prop, p, val) {
+ if (i >= ARRAY_SIZE(div_data))
+ break;
+ if (val == 1)
+ vc3->clk_div[i].flags = CLK_DIVIDER_READ_ONLY;
+
+ i++;
+ }
+}
+
+static void vc3_clk_flags_parse_dt(struct device *dev, u32 *crt_clks)
+{
+ struct device_node *np = dev->of_node;
+ struct property *prop;
+ const __be32 *p;
+ u32 i = 0;
+ u32 val;
+
+ of_property_for_each_u32(np, "renesas,clock-flags", prop, p, val) {
+ if (i >= ARRAY_SIZE(clk_out_data))
+ break;
+ *crt_clks++ = val;
+ i++;
+ }
+}
+
+static void vc3_fill_init_data(struct clk_init_data *init,
+ const struct vc3_clk_data *mux,
+ const struct clk_ops *ops,
+ u32 flags, int n,
+ const char **pll_parent_names,
+ const char **clkin_name)
+{
+ unsigned int i;
+
+ init->name = mux->name;
+ init->ops = ops;
+ init->flags = CLK_SET_RATE_PARENT;
+ init->num_parents = n;
+ for (i = 0; i < n; i++) {
+ if (!mux->parent_names[i])
+ pll_parent_names[i] = clkin_name[0];
+ else
+ pll_parent_names[i] = mux->parent_names[i];
+ }
+
+ init->parent_names = pll_parent_names;
+}
+
+static int vc3_clk_register(struct device *dev, struct vc3_driver_data *vc3,
+ struct vc3_hw_data *data, const void *clk_data,
+ struct clk_init_data *init, int n)
+{
+ data->hw.init = init;
+ data->num = n;
+ data->vc3 = vc3;
+ data->data = clk_data;
+
+ return devm_clk_hw_register(dev, &data->hw);
+}
+
+static int vc3_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ u8 settings[NUM_CONFIG_REGISTERS];
+ const char *pll_parent_names[3];
+ struct vc3_driver_data *vc3;
+ const char *clkin_name[1];
+ struct clk_init_data init;
+ u32 crit_clks[6] = {};
+ struct clk *clk;
+ int ret, i;
+
+ vc3 = devm_kzalloc(dev, sizeof(*vc3), GFP_KERNEL);
+ if (!vc3)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, vc3);
+ vc3->client = client;
+
+ vc3->regmap = devm_regmap_init_i2c(client, &vc3_regmap_config);
+ if (IS_ERR(vc3->regmap))
+ return dev_err_probe(dev, PTR_ERR(vc3->regmap),
+ "failed to allocate register map\n");
+
+ ret = of_property_read_u8_array(dev->of_node, "renesas,settings",
+ settings, ARRAY_SIZE(settings));
+ if (!ret) {
+ /*
+ * A raw settings array was specified in the DT. Write the
+ * settings to the device immediately.
+ */
+ for (i = 0; i < NUM_CONFIG_REGISTERS; i++) {
+ ret = regmap_write(vc3->regmap, i, settings[i]);
+ if (ret) {
+ dev_err(dev, "error writing to chip (%i)\n", ret);
+ return ret;
+ }
+ }
+ } else if (ret == -EOVERFLOW) {
+ dev_err(&client->dev, "EOVERFLOW reg settings. ARRAY_SIZE: %zu",
+ ARRAY_SIZE(settings));
+ return ret;
+ }
+
+ /* Register clock ref */
+ memset(&init, 0, sizeof(init));
+
+ clk = devm_clk_get(dev, "x1");
+ if (PTR_ERR(clk) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ if (!IS_ERR(clk)) {
+ clkin_name[0] = __clk_get_name(clk);
+ } else {
+ clk = devm_clk_get(dev, "clkin");
+ if (PTR_ERR(clk) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ if (!IS_ERR(clk))
+ clkin_name[0] = __clk_get_name(clk);
+ }
+
+ if (IS_ERR_OR_NULL(clk))
+ return dev_err_probe(&client->dev, -EINVAL, "no input clk\n");
+
+ /* Register pfd muxes */
+ for (i = 0; i < ARRAY_SIZE(pfd_mux_data); i++) {
+ vc3_fill_init_data(&init, &pfd_mux_data[i], &vc3_pfd_mux_ops,
+ CLK_SET_RATE_PARENT, 2, pll_parent_names,
+ clkin_name);
+ ret = vc3_clk_register(dev, vc3, &vc3->clk_pfd_mux[i],
+ &pfd_mux_data[i], &init, i);
+ if (ret)
+ return dev_err_probe(dev, ret, "%s failed\n", init.name);
+ }
+
+ /* Register pfd's */
+ for (i = 0; i < ARRAY_SIZE(pfd_data); i++) {
+ if (i == VC3_PFD1)
+ pll_parent_names[0] = clkin_name[0];
+ else
+ pll_parent_names[0] = pfd_mux_data[i - 1].name;
+
+ init.name = pfd_data[i].name;
+ init.ops = &vc3_pfd_ops;
+ init.flags = CLK_SET_RATE_PARENT;
+ init.num_parents = 1;
+ init.parent_names = pll_parent_names;
+
+ ret = vc3_clk_register(dev, vc3, &vc3->clk_pfd[i],
+ &pfd_data[i], &init, i);
+ if (ret)
+ return dev_err_probe(dev, ret, "%s failed\n", init.name);
+ }
+
+ /* Register pll's */
+ for (i = 0; i < ARRAY_SIZE(pll_data); i++) {
+ pll_parent_names[0] = pfd_data[i].name;
+ init.name = pll_data[i].name;
+ init.ops = &vc3_pll_ops;
+ init.flags = CLK_SET_RATE_PARENT;
+ init.num_parents = 1;
+ init.parent_names = pll_parent_names;
+
+ ret = vc3_clk_register(dev, vc3, &vc3->clk_pll[i],
+ &pll_data[i], &init, i);
+ if (ret)
+ return dev_err_probe(dev, ret, "%s failed\n", init.name);
+ }
+
+ /* Register divider muxes */
+ for (i = 0; i < ARRAY_SIZE(div_mux_data); i++) {
+ vc3_fill_init_data(&init, &div_mux_data[i], &vc3_div_mux_ops,
+ CLK_SET_RATE_PARENT, 2, pll_parent_names,
+ clkin_name);
+ ret = vc3_clk_register(dev, vc3, &vc3->clk_div_mux[i],
+ &div_mux_data[i], &init, i);
+ if (ret)
+ return dev_err_probe(dev, ret, "%s failed\n", init.name);
+ }
+
+ vc3_divider_type_parse_dt(dev, vc3);
+ /* Register dividers */
+ for (i = 0; i < ARRAY_SIZE(div_data); i++) {
+ switch (i) {
+ case VC3_DIV1:
+ pll_parent_names[0] = div_mux_data[VC3_DIV1_MUX].name;
+ break;
+ case VC3_DIV2:
+ pll_parent_names[0] = pll_data[VC3_PLL1].name;
+ break;
+ case VC3_DIV3:
+ pll_parent_names[0] = div_mux_data[VC3_DIV3_MUX].name;
+ break;
+ case VC3_DIV4:
+ pll_parent_names[0] = div_mux_data[VC3_DIV4_MUX].name;
+ break;
+ case VC3_DIV5:
+ pll_parent_names[0] = pll_data[VC3_PLL3].name;
+ break;
+ }
+
+ init.name = div_data[i].name;
+ init.ops = &vc3_div_ops;
+ init.flags = CLK_SET_RATE_PARENT;
+ init.num_parents = 1;
+ init.parent_names = pll_parent_names;
+
+ ret = vc3_clk_register(dev, vc3, &vc3->clk_div[i],
+ &div_data[i], &init, i);
+ if (ret)
+ return dev_err_probe(dev, ret, "%s failed\n", init.name);
+ }
+
+ /* Register clk muxes */
+ for (i = 0; i < ARRAY_SIZE(clk_mux_data); i++) {
+ vc3_fill_init_data(&init, &clk_mux_data[i], &vc3_clk_mux_ops,
+ CLK_SET_RATE_PARENT, 2, pll_parent_names,
+ clkin_name);
+ ret = vc3_clk_register(dev, vc3, &vc3->clk_mux[i],
+ &clk_mux_data[i], &init, i);
+ if (ret)
+ return dev_err_probe(dev, ret, "%s failed\n", init.name);
+ }
+
+ /* Register clk outputs */
+ vc3_clk_flags_parse_dt(dev, crit_clks);
+ for (i = 0; i < ARRAY_SIZE(clk_out_data); i++) {
+ vc3_fill_init_data(&init, &clk_out_data[i], &vc3_clk_out_ops,
+ crit_clks[i], 1, pll_parent_names,
+ clkin_name);
+ ret = vc3_clk_register(dev, vc3, &vc3->clk_out[i],
+ &clk_out_data[i], &init, i);
+ if (ret)
+ return dev_err_probe(dev, ret, "%s failed\n", init.name);
+ }
+
+ ret = of_clk_add_hw_provider(client->dev.of_node, vc3_of_clk_get, vc3);
+ if (ret)
+ return dev_err_probe(dev, ret, "unable to add clk provider\n");
+
+ return ret;
+}
+
+static void vc3_remove(struct i2c_client *client)
+{
+ of_clk_del_provider(client->dev.of_node);
+}
+
+static const struct of_device_id dev_ids[] = {
+ { .compatible = "renesas,5p35023" },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, dev_ids);
+
+static struct i2c_driver vc3_driver = {
+ .driver = {
+ .name = "vc3",
+ .of_match_table = of_match_ptr(dev_ids),
+ },
+ .probe_new = vc3_probe,
+ .remove = vc3_remove,
+};
+module_i2c_driver(vc3_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_DESCRIPTION("Renesas VersaClock 3 driver");
+MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH RFC 3/3] arm64: dts: renesas: rzg2l-smarc: Use versa3 clk for audio mclk
2023-02-20 13:13 [PATCH RFC 0/3] Add Versa3 clock generator support Biju Das
2023-02-20 13:13 ` [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings Biju Das
2023-02-20 13:13 ` [PATCH RFC 2/3] drivers: clk: Add support for versa3 clock driver Biju Das
@ 2023-02-20 13:13 ` Biju Das
2023-02-20 13:18 ` [PATCH RFC 0/3] Add Versa3 clock generator support Biju Das
3 siblings, 0 replies; 26+ messages in thread
From: Biju Das @ 2023-02-20 13:13 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Biju Das, linux-clk, Geert Uytterhoeven, Fabrizio Castro,
linux-renesas-soc
Currently audio mclk uses a fixed clk 11.2896MHz(multiple of 44.1KHz).
Replace this fixed clk to programmable versa3 clk that can provide
2 rates 11.2896MHz and 12.2880(multiple of 48KHz) based on audio
sampling rate for the playback/record.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
.../boot/dts/renesas/rz-smarc-common.dtsi | 7 ----
arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi | 35 +++++++++++++++++++
2 files changed, 35 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/rz-smarc-common.dtsi b/arch/arm64/boot/dts/renesas/rz-smarc-common.dtsi
index 3962d47b3e59..d724f49bd067 100644
--- a/arch/arm64/boot/dts/renesas/rz-smarc-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/rz-smarc-common.dtsi
@@ -32,12 +32,6 @@ chosen {
stdout-path = "serial0:115200n8";
};
- audio_mclock: audio_mclock {
- compatible = "fixed-clock";
- #clock-cells = <0>;
- clock-frequency = <11289600>;
- };
-
snd_rzg2l: sound {
compatible = "simple-audio-card";
simple-audio-card,format = "i2s";
@@ -55,7 +49,6 @@ cpu_dai: simple-audio-card,cpu {
};
codec_dai: simple-audio-card,codec {
- clocks = <&audio_mclock>;
sound-dai = <&wm8978>;
};
};
diff --git a/arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi b/arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi
index e180a955b6ac..cfda0e656fd8 100644
--- a/arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi
@@ -16,12 +16,22 @@ aliases {
serial1 = &scif2;
i2c3 = &i2c3;
};
+
+ x1_x2: xtal {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <24000000>;
+ };
};
&cpu_dai {
sound-dai = <&ssi0>;
};
+&codec_dai {
+ clocks = <&versa3 3>;
+};
+
&i2c3 {
pinctrl-0 = <&i2c3_pins>;
pinctrl-names = "default";
@@ -29,6 +39,31 @@ &i2c3 {
status = "okay";
+ versa3: versa3@68 {
+ compatible = "renesas,5p35023";
+ reg = <0x68>;
+ #clock-cells = <1>;
+ clocks = <&x1_x2>;
+ clock-names = "x1";
+ renesas,settings = [
+ 80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
+ 00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
+ 80 b0 45 c4 95
+ ];
+ assigned-clocks = <&versa3 0>,
+ <&versa3 1>,
+ <&versa3 2>,
+ <&versa3 3>,
+ <&versa3 4>,
+ <&versa3 5>;
+ assigned-clock-rates = <12288000>, <25000000>,
+ <12000000>, <11289600>,
+ <11289600>, <24000000>;
+ renesas,clock-divider-read-only = <1>, <1>, <1>, <1>, <1>;
+ renesas,clock-flags = <2176>, <2176>, <2176>, <2052>,
+ <2176>, <2048>;
+ };
+
wm8978: codec@1a {
compatible = "wlf,wm8978";
#sound-dai-cells = <0>;
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 0/3] Add Versa3 clock generator support
2023-02-20 13:13 [PATCH RFC 0/3] Add Versa3 clock generator support Biju Das
` (2 preceding siblings ...)
2023-02-20 13:13 ` [PATCH RFC 3/3] arm64: dts: renesas: rzg2l-smarc: Use versa3 clk for audio mclk Biju Das
@ 2023-02-20 13:18 ` Biju Das
3 siblings, 0 replies; 26+ messages in thread
From: Biju Das @ 2023-02-20 13:18 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Rob Herring, Krzysztof Kozlowski
Cc: linux-clk, Geert Uytterhoeven, Kuninori Morimoto,
Prabhakar Mahadev Lad, Fabrizio Castro, Mark Brown, Takashi Iwai,
alsa-devel, linux-renesas-soc,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
+ Rob , Krzysztof Kozlowski
> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: Monday, February 20, 2023 1:13 PM
> To: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
> <sboyd@kernel.org>
> Cc: Biju Das <biju.das.jz@bp.renesas.com>; linux-clk@vger.kernel.org; Geert
> Uytterhoeven <geert+renesas@glider.be>; Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>; Fabrizio Castro
> <fabrizio.castro.jz@renesas.com>; Mark Brown <broonie@kernel.org>; Takashi
> Iwai <tiwai@suse.de>; alsa-devel@alsa-project.org; linux-renesas-
> soc@vger.kernel.org
> Subject: [PATCH RFC 0/3] Add Versa3 clock generator support
>
> The 5P35023 is a VersaClock programmable clock generator and it provides 6
> clk outputs {diff2, diff1, se3, se2, se1 and refin}.
>
> It has an internal OTP memory allows the user to store the configuration in
> the device. After power up, the user can change the device register settings
> through the I2C interface when I2C mode is selected.
>
> This driver is for overriding OTP default values during boot based on a full
> register map from DT, and also minimal support to change the parent of a
> output clock.
>
> The motivation for developing this driver is for supporting 48KHz
> playback/record with audio codec on RZ/G2L SMARC EVK.
>
> On RZ/G2L SMARC EVK, By default audio mclk is connected to
> 11.2896 MHz clk which is multiple of 44.1KHz.
>
> Please see the below default OTP configuration of Dividers connected to
> output clocks.
>
> DIV3 12.2880 MHz DIFF2--> Audio clk2
> DIV5 11.2896 MHz SE1 --> Audio clk1
> DIV5 11.2896 MHz SE2 --> Audio mck
> DIV4 12 MHz SE3 --> This clk Not used
> DIV1 25 MHz DIFF1-->Ethernet clk
> Ref1-> 24MHz
>
> With this setup, we won't be able to do 48KHz playback/record on audio
> codec, as mck is always connected to 11.2896MHz clk.
>
> But by programming the i2c, we can make use of DIV4 to generate 12.2880 MHz
> and make that as parent of SE2 and there by supporting 48KHz
> playback/record.
>
> A block diagram with modification can be find here[1]
> [1] https://paste.pics/a253ce7cdc8720c3b5eb6953b97b25ff
>
> DIV3 12.2880 MHz DIFF2--> Audio clk2
> DIV5 11.2896 MHz SE1 --> Audio clk1
> DIV5 11.2896 MHz | SE2 --> Audio mck
> DIV4 12.2880 MHz |
> DIV2 12 MHz SE3 --> This clk Not used
> DIV1 25 MHz DIFF1--> Ethernet clk
> Ref1-> 24MHz
>
> The driver can read a full register map from the DT, and will use that
> register map to initialize the clk generator when the system boots.
> and later, based on sampling rate, it switches the parent of SE2 and support
> both 44.1 and 48 KHz playback/record at run time.
>
> 48KHz playback
> 1f: f6 --> setting Div4 as clock source for se2 Read at address 0x10049C00
> : 0x300B4022 --> Setting Audio clk2 in SSI
> pfd2 1 1 0 24000000
> pll2 1 1 0 491519897
> div4_mux 1 1 0 491519897
> div4 1 1 0 12287998
> se2_mux 1 1 0 12287998
> se2 1 1 0 12287998
>
> 44.1KHz playback
> 1f: b6 --> setting Div5 as clock source for se2 Read at address 0x10049C00:
> 0x700B4022--> Setting Audio clk1 in SSI
> pfd3_mux 1 1 0 24000000
> pfd3 1 1 0 960000
> pll3 1 1 0 564480000
> div5 1 1 0 11289600
> se2_mux 1 1 0 11289600
> se2 1 1 0 11289600
>
> Please provide your valuable comment for this patch series.
>
> Biju Das (3):
> dt-bindings: clock: Add Renesas versa3 clock generator bindings
> drivers: clk: Add support for versa3 clock driver
> arm64: dts: renesas: rzg2l-smarc: Use versa3 clk for audio mclk
>
> .../bindings/clock/renesas,versaclock3.yaml | 135 ++
> .../boot/dts/renesas/rz-smarc-common.dtsi | 7 -
> arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi | 35 +
> drivers/clk/Kconfig | 9 +
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-versaclock3.c | 1134 +++++++++++++++++
> 6 files changed, 1314 insertions(+), 7 deletions(-) create mode 100644
> Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> create mode 100644 drivers/clk/clk-versaclock3.c
>
> --
> 2.25.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-02-20 13:13 ` [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings Biju Das
@ 2023-02-22 9:34 ` Krzysztof Kozlowski
2023-03-08 14:39 ` Biju Das
0 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-22 9:34 UTC (permalink / raw)
To: Biju Das, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
On 20/02/2023 14:13, Biju Das wrote:
> Document Renesas versa3 clock generator(5P35023) bindings.
>
> The 5P35023 is a VersaClock programmable clock generator and
> is designed for low-power, consumer, and high-performance PCI
> Express applications. The 5P35023 device is a three PLL
> architecture design, and each PLL is individually programmable
> and allowing for up to 6 unique frequency outputs.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> .../bindings/clock/renesas,versaclock3.yaml | 135 ++++++++++++++++++
> 1 file changed, 135 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
>
> diff --git a/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> new file mode 100644
> index 000000000000..f45b8da73ec3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
Filename usually is based on the compatible. Why these two are so different?
> @@ -0,0 +1,135 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/renesas,versaclock3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas VersaClock 3 programmable I2C clock generators
> +
> +description: |
> + The 5P35023 is a VersaClock programmable clock generator and
> + is designed for low-power, consumer, and high-performance PCI
> + express applications. The 5P35023 device is a three PLL
> + architecture design, and each PLL is individually programmable
> + and allowing for up to 6 unique frequency outputs.
> +
> + An internal OTP memory allows the user to store the configuration
> + in the device. After power up, the user can change the device register
> + settings through the I2C interface when I2C mode is selected.
> +
> + The driver can read a full register map from the DT, and will use that
> + register map to initialize the attached part (via I2C) when the system
> + boots. Any configuration not supported by the common clock framework
> + must be done via the full register map, including optimized settings.
> +
> + Link to datasheet: https://www.renesas.com/us/en/products/clocks-timing/
> + clock-generation/programmable-clocks/
> + 5p35023-versaclock-3s-programmable-clock-generator
I think link should not be wrapped. Start in next line and just make it
long.
While touching this, please keep the same order of entries as in
example-schema, so maintainers go after title.
> +
> +maintainers:
> + - Biju Das <biju.das.jz@bp.renesas.com>
> +
> +properties:
> + compatible:
> + enum:
> + - renesas,5p35023
> +
> + reg:
> + description: I2C device address
Drop description, it's obvious.
> + maxItems: 1
> +
> + '#clock-cells':
> + const: 1
> +
> + clock-names:
> + oneOf:
> + - items:
> + - const: x1
> + - items:
> + - const: clkin
This should be specific, not one or another. Why do you have two
entirely different clock inputs?
(and if this stays, then just items: - enum: [])
> +
> + clocks:
> + maxItems: 1
> +
> + renesas,settings:
> + description: Optional, complete register map of the device.
> + Optimized settings for the device must be provided in full
> + and are written during initialization.
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> + minItems: 37
maxItems instead... but I am not sure that we allow register settings in
DT in general.
> +
> + assigned-clocks:
> + minItems: 6
Drop.
> +
> + assigned-clock-rates:
> + minItems: 6
Drop.
> +
> + renesas,clock-divider-read-only:
> + description: Flag for setting divider in read only mode.
Flag? Then type: boolean.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 5
This is broken...
> +
> + renesas,clock-flags:
> + description: Flags used in common clock frame work for configuring
> + clk outputs. See include/linux/clk-provider.h
These are not bindings, so why do you non-bindings constants as
bindings? They can change anytime. Choose one:
1. Drop entire property,
2. Make it a proper binding property, so an ABI and explain why this is
DT specific. None of clock providers have to do it, so you need here
good explanation.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 6
maxItems instead
> +
> +required:
> + - compatible
> + - reg
> + - '#clock-cells'
> + - clocks
> + - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + /* 24MHz crystal */
> + x1_x2: xtal {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <24000000>;
> + };
Drop this part, obvious.
> +
> + i2c@0 {
> + reg = <0x0 0x100>;
Just i2c { and drop reg
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + versa3: clock-generator@68 {
> + compatible = "renesas,5p35023";
> + reg = <0x68>;
> + #clock-cells = <1>;
> +
> + clocks = <&x1_x2>;
> + clock-names = "x1";
> +
> + renesas,settings = [
> + 80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
> + 00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
> + 80 b0 45 c4 95
> + ];
> +
> + assigned-clocks = <&versa3 0>,
> + <&versa3 1>,
> + <&versa3 2>,
> + <&versa3 3>,
> + <&versa3 4>,
> + <&versa3 5>;
> + assigned-clock-rates = <12288000>, <25000000>,
> + <12000000>, <11289600>,
> + <11289600>, <24000000>;
> + renesas,clock-divider-read-only = <1>, <1>, <1>, <1>, <1>;
> + renesas,clock-flags = <2176>, <2176>, <2176>, <2052>,
> + <2176>, <2048>;
> + };
> + };
> +
> + /* Consumer referencing the versa 3 */
> + consumer {
> + /* ... */
> + clocks = <&versa3 3>;
> + /* ... */
Drop consumer. Do you see it anywhere else?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 2/3] drivers: clk: Add support for versa3 clock driver
2023-02-20 13:13 ` [PATCH RFC 2/3] drivers: clk: Add support for versa3 clock driver Biju Das
@ 2023-03-06 20:22 ` Stephen Boyd
2023-03-09 12:25 ` Biju Das
0 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2023-03-06 20:22 UTC (permalink / raw)
To: Biju Das, Michael Turquette
Cc: Biju Das, linux-clk, Geert Uytterhoeven, Fabrizio Castro,
linux-renesas-soc
Quoting Biju Das (2023-02-20 05:13:06)
> diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c
> new file mode 100644
> index 000000000000..561cfad8a243
> --- /dev/null
> +++ b/drivers/clk/clk-versaclock3.c
> @@ -0,0 +1,1134 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Renesas Versaclock 3
> + *
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + */
> +
> +#include <linux/clk.h>
Please remove this include after moving to 'struct clk_parent_data'.
> +#include <linux/clk-provider.h>
> +#include <linux/i2c.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#define NUM_CONFIG_REGISTERS 37
> +
[...]
> +
> +static void vc3_clk_flags_parse_dt(struct device *dev, u32 *crt_clks)
> +{
> + struct device_node *np = dev->of_node;
> + struct property *prop;
> + const __be32 *p;
> + u32 i = 0;
> + u32 val;
> +
> + of_property_for_each_u32(np, "renesas,clock-flags", prop, p, val) {
> + if (i >= ARRAY_SIZE(clk_out_data))
> + break;
> + *crt_clks++ = val;
> + i++;
> + }
> +}
> +
> +static void vc3_fill_init_data(struct clk_init_data *init,
> + const struct vc3_clk_data *mux,
> + const struct clk_ops *ops,
> + u32 flags, int n,
> + const char **pll_parent_names,
> + const char **clkin_name)
> +{
> + unsigned int i;
> +
> + init->name = mux->name;
> + init->ops = ops;
> + init->flags = CLK_SET_RATE_PARENT;
> + init->num_parents = n;
> + for (i = 0; i < n; i++) {
> + if (!mux->parent_names[i])
> + pll_parent_names[i] = clkin_name[0];
> + else
> + pll_parent_names[i] = mux->parent_names[i];
Instead of string names please use clk_hw pointers or 'struct
clk_parent_data'.
> + }
> +
> + init->parent_names = pll_parent_names;
> +}
> +
> +static int vc3_clk_register(struct device *dev, struct vc3_driver_data *vc3,
> + struct vc3_hw_data *data, const void *clk_data,
> + struct clk_init_data *init, int n)
const init pointer?
> +{
> + data->hw.init = init;
> + data->num = n;
> + data->vc3 = vc3;
> + data->data = clk_data;
> +
> + return devm_clk_hw_register(dev, &data->hw);
> +}
> +
> +static int vc3_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + u8 settings[NUM_CONFIG_REGISTERS];
> + const char *pll_parent_names[3];
> + struct vc3_driver_data *vc3;
> + const char *clkin_name[1];
> + struct clk_init_data init;
> + u32 crit_clks[6] = {};
> + struct clk *clk;
> + int ret, i;
> +
> + vc3 = devm_kzalloc(dev, sizeof(*vc3), GFP_KERNEL);
> + if (!vc3)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, vc3);
> + vc3->client = client;
> +
> + vc3->regmap = devm_regmap_init_i2c(client, &vc3_regmap_config);
> + if (IS_ERR(vc3->regmap))
> + return dev_err_probe(dev, PTR_ERR(vc3->regmap),
> + "failed to allocate register map\n");
> +
> + ret = of_property_read_u8_array(dev->of_node, "renesas,settings",
> + settings, ARRAY_SIZE(settings));
> + if (!ret) {
> + /*
> + * A raw settings array was specified in the DT. Write the
> + * settings to the device immediately.
> + */
> + for (i = 0; i < NUM_CONFIG_REGISTERS; i++) {
> + ret = regmap_write(vc3->regmap, i, settings[i]);
> + if (ret) {
> + dev_err(dev, "error writing to chip (%i)\n", ret);
> + return ret;
> + }
> + }
> + } else if (ret == -EOVERFLOW) {
> + dev_err(&client->dev, "EOVERFLOW reg settings. ARRAY_SIZE: %zu",
> + ARRAY_SIZE(settings));
> + return ret;
> + }
> +
> + /* Register clock ref */
> + memset(&init, 0, sizeof(init));
> +
> + clk = devm_clk_get(dev, "x1");
> + if (PTR_ERR(clk) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + if (!IS_ERR(clk)) {
> + clkin_name[0] = __clk_get_name(clk);
> + } else {
> + clk = devm_clk_get(dev, "clkin");
> + if (PTR_ERR(clk) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + if (!IS_ERR(clk))
> + clkin_name[0] = __clk_get_name(clk);
> + }
Please use 'struct clk_parent_data' instead of clk consumer APIs.
> +
> + if (IS_ERR_OR_NULL(clk))
> + return dev_err_probe(&client->dev, -EINVAL, "no input clk\n");
> +
> + /* Register pfd muxes */
> + for (i = 0; i < ARRAY_SIZE(pfd_mux_data); i++) {
> + vc3_fill_init_data(&init, &pfd_mux_data[i], &vc3_pfd_mux_ops,
> + CLK_SET_RATE_PARENT, 2, pll_parent_names,
> + clkin_name);
> + ret = vc3_clk_register(dev, vc3, &vc3->clk_pfd_mux[i],
> + &pfd_mux_data[i], &init, i);
> + if (ret)
> + return dev_err_probe(dev, ret, "%s failed\n", init.name);
> + }
> +
> + /* Register pfd's */
> + for (i = 0; i < ARRAY_SIZE(pfd_data); i++) {
> + if (i == VC3_PFD1)
> + pll_parent_names[0] = clkin_name[0];
> + else
> + pll_parent_names[0] = pfd_mux_data[i - 1].name;
> +
> + init.name = pfd_data[i].name;
> + init.ops = &vc3_pfd_ops;
> + init.flags = CLK_SET_RATE_PARENT;
> + init.num_parents = 1;
> + init.parent_names = pll_parent_names;
> +
> + ret = vc3_clk_register(dev, vc3, &vc3->clk_pfd[i],
> + &pfd_data[i], &init, i);
> + if (ret)
> + return dev_err_probe(dev, ret, "%s failed\n", init.name);
> + }
> +
> + /* Register pll's */
> + for (i = 0; i < ARRAY_SIZE(pll_data); i++) {
> + pll_parent_names[0] = pfd_data[i].name;
> + init.name = pll_data[i].name;
> + init.ops = &vc3_pll_ops;
> + init.flags = CLK_SET_RATE_PARENT;
> + init.num_parents = 1;
> + init.parent_names = pll_parent_names;
> +
> + ret = vc3_clk_register(dev, vc3, &vc3->clk_pll[i],
> + &pll_data[i], &init, i);
> + if (ret)
> + return dev_err_probe(dev, ret, "%s failed\n", init.name);
> + }
> +
> + /* Register divider muxes */
> + for (i = 0; i < ARRAY_SIZE(div_mux_data); i++) {
> + vc3_fill_init_data(&init, &div_mux_data[i], &vc3_div_mux_ops,
> + CLK_SET_RATE_PARENT, 2, pll_parent_names,
> + clkin_name);
> + ret = vc3_clk_register(dev, vc3, &vc3->clk_div_mux[i],
> + &div_mux_data[i], &init, i);
> + if (ret)
> + return dev_err_probe(dev, ret, "%s failed\n", init.name);
> + }
> +
> + vc3_divider_type_parse_dt(dev, vc3);
> + /* Register dividers */
> + for (i = 0; i < ARRAY_SIZE(div_data); i++) {
> + switch (i) {
> + case VC3_DIV1:
> + pll_parent_names[0] = div_mux_data[VC3_DIV1_MUX].name;
> + break;
> + case VC3_DIV2:
> + pll_parent_names[0] = pll_data[VC3_PLL1].name;
> + break;
> + case VC3_DIV3:
> + pll_parent_names[0] = div_mux_data[VC3_DIV3_MUX].name;
> + break;
> + case VC3_DIV4:
> + pll_parent_names[0] = div_mux_data[VC3_DIV4_MUX].name;
> + break;
> + case VC3_DIV5:
> + pll_parent_names[0] = pll_data[VC3_PLL3].name;
> + break;
> + }
> +
> + init.name = div_data[i].name;
> + init.ops = &vc3_div_ops;
> + init.flags = CLK_SET_RATE_PARENT;
> + init.num_parents = 1;
> + init.parent_names = pll_parent_names;
> +
> + ret = vc3_clk_register(dev, vc3, &vc3->clk_div[i],
> + &div_data[i], &init, i);
> + if (ret)
> + return dev_err_probe(dev, ret, "%s failed\n", init.name);
> + }
> +
> + /* Register clk muxes */
> + for (i = 0; i < ARRAY_SIZE(clk_mux_data); i++) {
> + vc3_fill_init_data(&init, &clk_mux_data[i], &vc3_clk_mux_ops,
> + CLK_SET_RATE_PARENT, 2, pll_parent_names,
> + clkin_name);
> + ret = vc3_clk_register(dev, vc3, &vc3->clk_mux[i],
> + &clk_mux_data[i], &init, i);
> + if (ret)
> + return dev_err_probe(dev, ret, "%s failed\n", init.name);
> + }
> +
> + /* Register clk outputs */
> + vc3_clk_flags_parse_dt(dev, crit_clks);
> + for (i = 0; i < ARRAY_SIZE(clk_out_data); i++) {
> + vc3_fill_init_data(&init, &clk_out_data[i], &vc3_clk_out_ops,
> + crit_clks[i], 1, pll_parent_names,
> + clkin_name);
> + ret = vc3_clk_register(dev, vc3, &vc3->clk_out[i],
> + &clk_out_data[i], &init, i);
> + if (ret)
> + return dev_err_probe(dev, ret, "%s failed\n", init.name);
> + }
> +
> + ret = of_clk_add_hw_provider(client->dev.of_node, vc3_of_clk_get, vc3);
Can you use devm?
> + if (ret)
> + return dev_err_probe(dev, ret, "unable to add clk provider\n");
> +
> + return ret;
> +}
> +
> +static void vc3_remove(struct i2c_client *client)
> +{
> + of_clk_del_provider(client->dev.of_node);
> +}
Using devm means this whole function can be removed.
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-02-22 9:34 ` Krzysztof Kozlowski
@ 2023-03-08 14:39 ` Biju Das
2023-03-08 14:50 ` Geert Uytterhoeven
2023-03-08 18:47 ` Krzysztof Kozlowski
0 siblings, 2 replies; 26+ messages in thread
From: Biju Das @ 2023-03-08 14:39 UTC (permalink / raw)
To: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
Hi Krzysztof Kozlowski,
Thanks for the feedback.
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, February 22, 2023 9:34 AM
> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; linux-renesas-
> soc@vger.kernel.org; linux-clk@vger.kernel.org; devicetree@vger.kernel.org;
> Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
>
> On 20/02/2023 14:13, Biju Das wrote:
> > Document Renesas versa3 clock generator(5P35023) bindings.
> >
> > The 5P35023 is a VersaClock programmable clock generator and is
> > designed for low-power, consumer, and high-performance PCI Express
> > applications. The 5P35023 device is a three PLL architecture design,
> > and each PLL is individually programmable and allowing for up to 6
> > unique frequency outputs.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > .../bindings/clock/renesas,versaclock3.yaml | 135 ++++++++++++++++++
> > 1 file changed, 135 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> > b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> > new file mode 100644
> > index 000000000000..f45b8da73ec3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
>
> Filename usually is based on the compatible. Why these two are so different?
Versa3 clock generator has the following variants.
5P35023, 5L35021, 5L35023 and 5P35021
RZ/G2L SMARC EVK uses 5P35023. So I used generic one as file name.
And added compatible for specific one.
>
>
> > +title: Renesas VersaClock 3 programmable I2C clock generators
> > +
> > +description: |
> > + The 5P35023 is a VersaClock programmable clock generator and
> > + is designed for low-power, consumer, and high-performance PCI
> > + express applications. The 5P35023 device is a three PLL
> > + architecture design, and each PLL is individually programmable
> > + and allowing for up to 6 unique frequency outputs.
> > +
> > + An internal OTP memory allows the user to store the configuration
> > + in the device. After power up, the user can change the device
> > + register settings through the I2C interface when I2C mode is selected.
> > +
> > + The driver can read a full register map from the DT, and will use
> > + that register map to initialize the attached part (via I2C) when
> > + the system boots. Any configuration not supported by the common
> > + clock framework must be done via the full register map, including
> optimized settings.
> > +
> > + Link to datasheet: https://www.renesas.com/us/en/products/clocks-
> timing/
> > + clock-generation/programmable-clocks/
> > +
> > + 5p35023-versaclock-3s-programmable-clock-generator
>
> I think link should not be wrapped. Start in next line and just make it
> long.
OK.
>
> While touching this, please keep the same order of entries as in example-
> schema, so maintainers go after title.
Agreed.
>
> > +
> > +maintainers:
> > + - Biju Das <biju.das.jz@bp.renesas.com>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - renesas,5p35023
> > +
> > + reg:
> > + description: I2C device address
>
> Drop description, it's obvious.
OK.
>
> > + maxItems: 1
> > +
> > + '#clock-cells':
> > + const: 1
> > +
> > + clock-names:
> > + oneOf:
> > + - items:
> > + - const: x1
> > + - items:
> > + - const: clkin
>
> This should be specific, not one or another. Why do you have two entirely
> different clock inputs?
Reference input can be Crystal oscillator interface input(x1) or differential
clock input pin(clkin)
>
> (and if this stays, then just items: - enum: [])
OK, I will use enum.
>
>
> > +
> > + clocks:
> > + maxItems: 1
> > +
> > + renesas,settings:
> > + description: Optional, complete register map of the device.
> > + Optimized settings for the device must be provided in full
> > + and are written during initialization.
> > + $ref: /schemas/types.yaml#/definitions/uint8-array
> > + minItems: 37
>
> maxItems instead... but I am not sure that we allow register settings in DT
> in general.
Agreed. I guess it is allowed [1]
[1] https://lore.kernel.org/linux-renesas-soc/833d3837892f0879233695636291af97a746e584.1643968653.git.michal.simek@xilinx.com/
>
> > +
> > + assigned-clocks:
> > + minItems: 6
>
> Drop.
OK.
>
> > +
> > + assigned-clock-rates:
> > + minItems: 6
>
> Drop.
OK.
>
> > +
> > + renesas,clock-divider-read-only:
> > + description: Flag for setting divider in read only mode.
>
> Flag? Then type: boolean.
Agreed.
>
> > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > + minItems: 5
>
> This is broken...
OK you mean maxItems. Based on Boolean type I will update this
>
> > +
> > + renesas,clock-flags:
> > + description: Flags used in common clock frame work for configuring
> > + clk outputs. See include/linux/clk-provider.h
>
> These are not bindings, so why do you non-bindings constants as bindings?
> They can change anytime. Choose one:
> 1. Drop entire property,
> 2. Make it a proper binding property, so an ABI and explain why this is DT
> specific. None of clock providers have to do it, so you need here good
> explanation.
I will choose 2 and will explain as user should be allowed to
configure the output clock from clk generator, so that it has flexibility
for
1) changing the rates (propagate rate change up one level)
2) fixed always
3) don't gate the ouput clk at all.
>
> > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > + minItems: 6
>
> maxItems instead
OK.
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - '#clock-cells'
> > + - clocks
> > + - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + /* 24MHz crystal */
> > + x1_x2: xtal {
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <24000000>;
> > + };
>
> Drop this part, obvious.
OK.
>
> > +
> > + i2c@0 {
> > + reg = <0x0 0x100>;
>
> Just i2c { and drop reg
Agreed.
>
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + versa3: clock-generator@68 {
> > + compatible = "renesas,5p35023";
> > + reg = <0x68>;
> > + #clock-cells = <1>;
> > +
> > + clocks = <&x1_x2>;
> > + clock-names = "x1";
> > +
> > + renesas,settings = [
> > + 80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
> > + 00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
> > + 80 b0 45 c4 95
> > + ];
> > +
> > + assigned-clocks = <&versa3 0>,
> > + <&versa3 1>,
> > + <&versa3 2>,
> > + <&versa3 3>,
> > + <&versa3 4>,
> > + <&versa3 5>;
> > + assigned-clock-rates = <12288000>, <25000000>,
> > + <12000000>, <11289600>,
> > + <11289600>, <24000000>;
> > + renesas,clock-divider-read-only = <1>, <1>, <1>, <1>, <1>;
> > + renesas,clock-flags = <2176>, <2176>, <2176>, <2052>,
> > + <2176>, <2048>;
> > + };
> > + };
> > +
> > + /* Consumer referencing the versa 3 */
> > + consumer {
> > + /* ... */
> > + clocks = <&versa3 3>;
> > + /* ... */
>
> Drop consumer. Do you see it anywhere else?
No, will drop.
Cheers,
Biju
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-08 14:39 ` Biju Das
@ 2023-03-08 14:50 ` Geert Uytterhoeven
2023-03-08 14:57 ` Biju Das
2023-03-08 18:47 ` Krzysztof Kozlowski
1 sibling, 1 reply; 26+ messages in thread
From: Geert Uytterhoeven @ 2023-03-08 14:50 UTC (permalink / raw)
To: Biju Das
Cc: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Geert Uytterhoeven,
linux-renesas-soc, linux-clk, devicetree, Fabrizio Castro
Hi Biju,
On Wed, Mar 8, 2023 at 3:39 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > On 20/02/2023 14:13, Biju Das wrote:
> > > Document Renesas versa3 clock generator(5P35023) bindings.
> > >
> > > The 5P35023 is a VersaClock programmable clock generator and is
> > > designed for low-power, consumer, and high-performance PCI Express
> > > applications. The 5P35023 device is a three PLL architecture design,
> > > and each PLL is individually programmable and allowing for up to 6
> > > unique frequency outputs.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
> > > + clock-names:
> > > + oneOf:
> > > + - items:
> > > + - const: x1
> > > + - items:
> > > + - const: clkin
> >
> > This should be specific, not one or another. Why do you have two entirely
> > different clock inputs?
>
> Reference input can be Crystal oscillator interface input(x1) or differential
> clock input pin(clkin)
I believe that's purely a hardware feature, which does not need any
software configuration?
I.e. logically, there's just a single clock input, i.e. no need for clock-names.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-08 14:50 ` Geert Uytterhoeven
@ 2023-03-08 14:57 ` Biju Das
0 siblings, 0 replies; 26+ messages in thread
From: Biju Das @ 2023-03-08 14:57 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Geert Uytterhoeven,
linux-renesas-soc, linux-clk, devicetree, Fabrizio Castro
Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
>
> Hi Biju,
>
> On Wed, Mar 8, 2023 at 3:39 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> On
> > > 20/02/2023 14:13, Biju Das wrote:
> > > > Document Renesas versa3 clock generator(5P35023) bindings.
> > > >
> > > > The 5P35023 is a VersaClock programmable clock generator and is
> > > > designed for low-power, consumer, and high-performance PCI Express
> > > > applications. The 5P35023 device is a three PLL architecture
> > > > design, and each PLL is individually programmable and allowing for
> > > > up to 6 unique frequency outputs.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.
> > > > +++ yaml
>
> > > > + clock-names:
> > > > + oneOf:
> > > > + - items:
> > > > + - const: x1
> > > > + - items:
> > > > + - const: clkin
> > >
> > > This should be specific, not one or another. Why do you have two
> > > entirely different clock inputs?
> >
> > Reference input can be Crystal oscillator interface input(x1) or
> > differential clock input pin(clkin)
>
> I believe that's purely a hardware feature, which does not need any software
> configuration?
> I.e. logically, there's just a single clock input, i.e. no need for clock-
> names.
OK. Agreed. Will remove clock-names.
Cheers,
Biju
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-08 14:39 ` Biju Das
2023-03-08 14:50 ` Geert Uytterhoeven
@ 2023-03-08 18:47 ` Krzysztof Kozlowski
2023-03-08 18:55 ` Biju Das
1 sibling, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-08 18:47 UTC (permalink / raw)
To: Biju Das, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
On 08/03/2023 15:39, Biju Das wrote:
>>> +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
>>
>> Filename usually is based on the compatible. Why these two are so different?
>
> Versa3 clock generator has the following variants.
>
> 5P35023, 5L35021, 5L35023 and 5P35021
>
> RZ/G2L SMARC EVK uses 5P35023. So I used generic one as file name.
> And added compatible for specific one.
And what about other devices? Since you do not add them, just keep
compatible as filename.
>>
>>> +
>>> + clocks:
>>> + maxItems: 1
>>> +
>>> + renesas,settings:
>>> + description: Optional, complete register map of the device.
>>> + Optimized settings for the device must be provided in full
>>> + and are written during initialization.
>>> + $ref: /schemas/types.yaml#/definitions/uint8-array
>>> + minItems: 37
>>
>> maxItems instead... but I am not sure that we allow register settings in DT
>> in general.
>
> Agreed. I guess it is allowed [1]
> [1] https://lore.kernel.org/linux-renesas-soc/833d3837892f0879233695636291af97a746e584.1643968653.git.michal.simek@xilinx.com/
I don't see Rob's review on this, so what do you prove exactly?
>
>>
>>> +
>>> + assigned-clocks:
>>> + minItems: 6
>>
>> Drop.
>
> OK.
>
>>
>>> +
>>> + assigned-clock-rates:
>>> + minItems: 6
>>
>> Drop.
>
> OK.
>
>>
>>> +
>>> + renesas,clock-divider-read-only:
>>> + description: Flag for setting divider in read only mode.
>>
>> Flag? Then type: boolean.
>
> Agreed.
>>
>>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>>> + minItems: 5
>>
>> This is broken...
> OK you mean maxItems. Based on Boolean type I will update this
I mean, it does not match the description. Maybe I just don't understand
here something, but flag is boolean. Anyway, minItems means you can have
million of items, so was it intended?
>>
>>> +
>>> + renesas,clock-flags:
>>> + description: Flags used in common clock frame work for configuring
>>> + clk outputs. See include/linux/clk-provider.h
>>
>> These are not bindings, so why do you non-bindings constants as bindings?
>> They can change anytime. Choose one:
>> 1. Drop entire property,
>> 2. Make it a proper binding property, so an ABI and explain why this is DT
>> specific. None of clock providers have to do it, so you need here good
>> explanation.
>
> I will choose 2 and will explain as user should be allowed to
> configure the output clock from clk generator, so that it has flexibility
> for
> 1) changing the rates (propagate rate change up one level)
> 2) fixed always
> 3) don't gate the ouput clk at all.
User's choice is task for user-space, so not a good explanation for DT.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-08 18:47 ` Krzysztof Kozlowski
@ 2023-03-08 18:55 ` Biju Das
2023-03-08 19:17 ` Krzysztof Kozlowski
0 siblings, 1 reply; 26+ messages in thread
From: Biju Das @ 2023-03-08 18:55 UTC (permalink / raw)
To: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
Hi Krzysztof Kozlowski,
Thanks for the feedback.
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
>
> On 08/03/2023 15:39, Biju Das wrote:
>
> >>> +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.ya
> >>> +++ ml
> >>
> >> Filename usually is based on the compatible. Why these two are so
> different?
> >
> > Versa3 clock generator has the following variants.
> >
> > 5P35023, 5L35021, 5L35023 and 5P35021
> >
> > RZ/G2L SMARC EVK uses 5P35023. So I used generic one as file name.
> > And added compatible for specific one.
>
> And what about other devices? Since you do not add them, just keep
> compatible as filename.
OK.
>
> >>
> >>> +
> >>> + clocks:
> >>> + maxItems: 1
> >>> +
> >>> + renesas,settings:
> >>> + description: Optional, complete register map of the device.
> >>> + Optimized settings for the device must be provided in full
> >>> + and are written during initialization.
> >>> + $ref: /schemas/types.yaml#/definitions/uint8-array
> >>> + minItems: 37
> >>
> >> maxItems instead... but I am not sure that we allow register settings
> >> in DT in general.
> >
> > Agreed. I guess it is allowed [1]
> > [1]
>
> I don't see Rob's review on this, so what do you prove exactly?
Subject: [PATCH v9 1/2] dt-bindings: Add binding for Renesas 8T49N241
Date: Fri, 4 Feb 2022 10:57:38 +0100 [thread overview]
Message-ID: <833d3837892f0879233695636291af97a746e584.1643968653.git.michal.simek@xilinx.com> (raw)
In-Reply-To: <cover.1643968653.git.michal.simek@xilinx.com>
From: Alex Helms <alexander.helms.jy@renesas.com>
Renesas 8T49N241 has 4 outputs, 1 integral and 3 fractional dividers.
The 8T49N241 accepts up to two differential or single-ended input clocks
and a fundamental-mode crystal input. The internal PLL can lock to either
of the input reference clocks or to the crystal to behave as a frequency
synthesizer.
Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
>
> >
> >>
> >>> +
> >>> + assigned-clocks:
> >>> + minItems: 6
> >>
> >> Drop.
> >
> > OK.
> >
> >>
> >>> +
> >>> + assigned-clock-rates:
> >>> + minItems: 6
> >>
> >> Drop.
> >
> > OK.
> >
> >>
> >>> +
> >>> + renesas,clock-divider-read-only:
> >>> + description: Flag for setting divider in read only mode.
> >>
> >> Flag? Then type: boolean.
> >
> > Agreed.
> >>
> >>> + $ref: /schemas/types.yaml#/definitions/uint32-array
> >>> + minItems: 5
> >>
> >> This is broken...
> > OK you mean maxItems. Based on Boolean type I will update this
>
> I mean, it does not match the description. Maybe I just don't understand
> here something, but flag is boolean. Anyway, minItems means you can have
> million of items, so was it intended?
It is a mistake.
>
> >>
> >>> +
> >>> + renesas,clock-flags:
> >>> + description: Flags used in common clock frame work for configuring
> >>> + clk outputs. See include/linux/clk-provider.h
> >>
> >> These are not bindings, so why do you non-bindings constants as bindings?
> >> They can change anytime. Choose one:
> >> 1. Drop entire property,
> >> 2. Make it a proper binding property, so an ABI and explain why this
> >> is DT specific. None of clock providers have to do it, so you need
> >> here good explanation.
> >
> > I will choose 2 and will explain as user should be allowed to
> > configure the output clock from clk generator, so that it has
> > flexibility for
> > 1) changing the rates (propagate rate change up one level)
> > 2) fixed always
> > 3) don't gate the ouput clk at all.
>
> User's choice is task for user-space, so not a good explanation for DT.
I don't think clock generator HW has any business with user space.
It is clk generator HW specific. Clk generator is vital component which
provides clocks to the system. We are providing some hardware feature which
is exposed as dt properties.
Like clock output is fixed rate clock or dynamic rate clock/
Cheers,
Biju
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-08 18:55 ` Biju Das
@ 2023-03-08 19:17 ` Krzysztof Kozlowski
2023-03-09 7:57 ` Biju Das
0 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-08 19:17 UTC (permalink / raw)
To: Biju Das, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
On 08/03/2023 19:55, Biju Das wrote:
>
> Renesas 8T49N241 has 4 outputs, 1 integral and 3 fractional dividers.
> The 8T49N241 accepts up to two differential or single-ended input clocks
> and a fundamental-mode crystal input. The internal PLL can lock to either
> of the input reference clocks or to the crystal to behave as a frequency
> synthesizer.
>
> Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
Ah, indeed, fine then.
>>
>>>>
>>>>> +
>>>>> + renesas,clock-flags:
>>>>> + description: Flags used in common clock frame work for configuring
>>>>> + clk outputs. See include/linux/clk-provider.h
>>>>
>>>> These are not bindings, so why do you non-bindings constants as bindings?
>>>> They can change anytime. Choose one:
>>>> 1. Drop entire property,
>>>> 2. Make it a proper binding property, so an ABI and explain why this
>>>> is DT specific. None of clock providers have to do it, so you need
>>>> here good explanation.
>>>
>>> I will choose 2 and will explain as user should be allowed to
>>> configure the output clock from clk generator, so that it has
>>> flexibility for
>>> 1) changing the rates (propagate rate change up one level)
>>> 2) fixed always
>>> 3) don't gate the ouput clk at all.
>>
>> User's choice is task for user-space, so not a good explanation for DT.
>
> I don't think clock generator HW has any business with user space.
>
> It is clk generator HW specific. Clk generator is vital component which
> provides clocks to the system.
Every clock controller is vital...
> We are providing some hardware feature which
> is exposed as dt properties.
>
> Like clock output is fixed rate clock or dynamic rate clock/
OK, I wait then for proper description which will explain and justify this.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-08 19:17 ` Krzysztof Kozlowski
@ 2023-03-09 7:57 ` Biju Das
2023-03-09 9:13 ` Krzysztof Kozlowski
0 siblings, 1 reply; 26+ messages in thread
From: Biju Das @ 2023-03-09 7:57 UTC (permalink / raw)
To: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
Hi Krzysztof Kozlowski,
Thanks for the feedback.
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
>
> On 08/03/2023 19:55, Biju Das wrote:
> >
> > Renesas 8T49N241 has 4 outputs, 1 integral and 3 fractional dividers.
> > The 8T49N241 accepts up to two differential or single-ended input
> > clocks and a fundamental-mode crystal input. The internal PLL can lock
> > to either of the input reference clocks or to the crystal to behave as
> > a frequency synthesizer.
> >
> > Signed-off-by: Alex Helms <alexander.helms.jy@renesas.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
>
> Ah, indeed, fine then.
>
> >>
> >>>>
> >>>>> +
> >>>>> + renesas,clock-flags:
> >>>>> + description: Flags used in common clock frame work for
> configuring
> >>>>> + clk outputs. See include/linux/clk-provider.h
> >>>>
> >>>> These are not bindings, so why do you non-bindings constants as
> bindings?
> >>>> They can change anytime. Choose one:
> >>>> 1. Drop entire property,
> >>>> 2. Make it a proper binding property, so an ABI and explain why
> >>>> this is DT specific. None of clock providers have to do it, so you
> >>>> need here good explanation.
> >>>
> >>> I will choose 2 and will explain as user should be allowed to
> >>> configure the output clock from clk generator, so that it has
> >>> flexibility for
> >>> 1) changing the rates (propagate rate change up one level)
> >>> 2) fixed always
> >>> 3) don't gate the ouput clk at all.
> >>
> >> User's choice is task for user-space, so not a good explanation for DT.
> >
> > I don't think clock generator HW has any business with user space.
> >
> > It is clk generator HW specific. Clk generator is vital component
> > which provides clocks to the system.
>
> Every clock controller is vital...
>
> > We are providing some hardware feature which is exposed as dt
> > properties.
> >
> > Like clock output is fixed rate clock or dynamic rate clock/
>
> OK, I wait then for proper description which will explain and justify this.
Here it is, Please let me know is it ok?
renesas,output-clock-fixed-rate-mode:
type: boolean
description:
In output clock fixed rate mode, the output clock frequency is always
fixed and the hardware will use the values from the OTP or full register
map initialized during boot.
If not given, the output clock rate is not fixed.
maxItems: 6
Cheers,
Biju
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 7:57 ` Biju Das
@ 2023-03-09 9:13 ` Krzysztof Kozlowski
2023-03-09 9:18 ` Biju Das
0 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-09 9:13 UTC (permalink / raw)
To: Biju Das, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
On 09/03/2023 08:57, Biju Das wrote:
>>> It is clk generator HW specific. Clk generator is vital component
>>> which provides clocks to the system.
>>
>> Every clock controller is vital...
>>
>>> We are providing some hardware feature which is exposed as dt
>>> properties.
>>>
>>> Like clock output is fixed rate clock or dynamic rate clock/
>>
>> OK, I wait then for proper description which will explain and justify this.
>
> Here it is, Please let me know is it ok?
>
> renesas,output-clock-fixed-rate-mode:
> type: boolean
> description:
> In output clock fixed rate mode, the output clock frequency is always
> fixed and the hardware will use the values from the OTP or full register
> map initialized during boot.
> If not given, the output clock rate is not fixed.
> maxItems: 6
boolean is scalar, not array, so no maxItems. If the frequency is taken
from OTP or register map, why they cannot also provide information the
clock is fixed?
>
> Cheers,
> Biju
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 9:13 ` Krzysztof Kozlowski
@ 2023-03-09 9:18 ` Biju Das
2023-03-09 9:44 ` Krzysztof Kozlowski
0 siblings, 1 reply; 26+ messages in thread
From: Biju Das @ 2023-03-09 9:18 UTC (permalink / raw)
To: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, March 9, 2023 9:14 AM
> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; linux-renesas-
> soc@vger.kernel.org; linux-clk@vger.kernel.org; devicetree@vger.kernel.org;
> Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
>
> On 09/03/2023 08:57, Biju Das wrote:
> >>> It is clk generator HW specific. Clk generator is vital component
> >>> which provides clocks to the system.
> >>
> >> Every clock controller is vital...
> >>
> >>> We are providing some hardware feature which is exposed as dt
> >>> properties.
> >>>
> >>> Like clock output is fixed rate clock or dynamic rate clock/
> >>
> >> OK, I wait then for proper description which will explain and justify
> this.
> >
> > Here it is, Please let me know is it ok?
> >
> > renesas,output-clock-fixed-rate-mode:
> > type: boolean
> > description:
> > In output clock fixed rate mode, the output clock frequency is
> always
> > fixed and the hardware will use the values from the OTP or full
> register
> > map initialized during boot.
> > If not given, the output clock rate is not fixed.
> > maxItems: 6
>
> boolean is scalar, not array, so no maxItems. If the frequency is taken from
> OTP or register map, why they cannot also provide information the clock is
> fixed?
OK, I will make an array property instead. From HW perspective each clock output from the
Clock generator is controllable ie, fixed rate or dynamic rate.
If all the output clocks are fixed rate one, then frequency is taken from OTP or
register map. But if any one clock output generates dynamic rate, then it uses
dynamic settings.
Cheers,
Biju
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 9:18 ` Biju Das
@ 2023-03-09 9:44 ` Krzysztof Kozlowski
2023-03-09 9:53 ` Biju Das
2023-03-09 9:58 ` Geert Uytterhoeven
0 siblings, 2 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-09 9:44 UTC (permalink / raw)
To: Biju Das, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
On 09/03/2023 10:18, Biju Das wrote:
>
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Thursday, March 9, 2023 9:14 AM
>> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
>> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob Herring
>> <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; linux-renesas-
>> soc@vger.kernel.org; linux-clk@vger.kernel.org; devicetree@vger.kernel.org;
>> Fabrizio Castro <fabrizio.castro.jz@renesas.com>
>> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
>> generator bindings
>>
>> On 09/03/2023 08:57, Biju Das wrote:
>>>>> It is clk generator HW specific. Clk generator is vital component
>>>>> which provides clocks to the system.
>>>>
>>>> Every clock controller is vital...
>>>>
>>>>> We are providing some hardware feature which is exposed as dt
>>>>> properties.
>>>>>
>>>>> Like clock output is fixed rate clock or dynamic rate clock/
>>>>
>>>> OK, I wait then for proper description which will explain and justify
>> this.
>>>
>>> Here it is, Please let me know is it ok?
>>>
>>> renesas,output-clock-fixed-rate-mode:
>>> type: boolean
>>> description:
>>> In output clock fixed rate mode, the output clock frequency is
>> always
>>> fixed and the hardware will use the values from the OTP or full
>> register
>>> map initialized during boot.
>>> If not given, the output clock rate is not fixed.
>>> maxItems: 6
>>
>> boolean is scalar, not array, so no maxItems. If the frequency is taken from
>> OTP or register map, why they cannot also provide information the clock is
>> fixed?
>
> OK, I will make an array property instead. From HW perspective each clock output from the
> Clock generator is controllable ie, fixed rate or dynamic rate.
>
> If all the output clocks are fixed rate one, then frequency is taken from OTP or
> register map. But if any one clock output generates dynamic rate, then it uses
> dynamic settings.
Second try, same question, let me know if it is not clear:
"why they cannot also provide information the clock is fixed?"
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 9:44 ` Krzysztof Kozlowski
@ 2023-03-09 9:53 ` Biju Das
2023-03-09 10:15 ` Krzysztof Kozlowski
2023-03-09 9:58 ` Geert Uytterhoeven
1 sibling, 1 reply; 26+ messages in thread
From: Biju Das @ 2023-03-09 9:53 UTC (permalink / raw)
To: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, March 9, 2023 9:44 AM
> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; linux-renesas-
> soc@vger.kernel.org; linux-clk@vger.kernel.org; devicetree@vger.kernel.org;
> Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
>
> On 09/03/2023 10:18, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Thursday, March 9, 2023 9:14 AM
> >> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
> >> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob
> >> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@linaro.org>
> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; linux-renesas-
> >> soc@vger.kernel.org; linux-clk@vger.kernel.org;
> >> devicetree@vger.kernel.org; Fabrizio Castro
> >> <fabrizio.castro.jz@renesas.com>
> >> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3
> >> clock generator bindings
> >>
> >> On 09/03/2023 08:57, Biju Das wrote:
> >>>>> It is clk generator HW specific. Clk generator is vital component
> >>>>> which provides clocks to the system.
> >>>>
> >>>> Every clock controller is vital...
> >>>>
> >>>>> We are providing some hardware feature which is exposed as dt
> >>>>> properties.
> >>>>>
> >>>>> Like clock output is fixed rate clock or dynamic rate clock/
> >>>>
> >>>> OK, I wait then for proper description which will explain and
> >>>> justify
> >> this.
> >>>
> >>> Here it is, Please let me know is it ok?
> >>>
> >>> renesas,output-clock-fixed-rate-mode:
> >>> type: boolean
> >>> description:
> >>> In output clock fixed rate mode, the output clock frequency is
> >> always
> >>> fixed and the hardware will use the values from the OTP or
> >>> full
> >> register
> >>> map initialized during boot.
> >>> If not given, the output clock rate is not fixed.
> >>> maxItems: 6
> >>
> >> boolean is scalar, not array, so no maxItems. If the frequency is
> >> taken from OTP or register map, why they cannot also provide
> >> information the clock is fixed?
> >
> > OK, I will make an array property instead. From HW perspective each
> > clock output from the Clock generator is controllable ie, fixed rate or
> dynamic rate.
> >
> > If all the output clocks are fixed rate one, then frequency is taken
> > from OTP or register map. But if any one clock output generates
> > dynamic rate, then it uses dynamic settings.
>
> Second try, same question, let me know if it is not clear:
>
> "why they cannot also provide information the clock is fixed?"
This information we are providing through dt.
It is a complex clock generator which provides 6 HW clock outputs.
The 6 HW clock outputs can be individually controllable to generate
Either fixed frequency or dynamic frequency.
Output clk1 "diff2",
Output clk2 "diff1",
Output clk3 "se3",
Output clk4 "se2",
Output clk5 "se1",
Output clk6 "ref"
I want to make "Output clk4" from clock generator as dynamic frequency one
And make other clock frequency from clock generator as fixed one.
How do you describe this in dt? Please share your thoughts.
Cheers,
Biju
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 9:44 ` Krzysztof Kozlowski
2023-03-09 9:53 ` Biju Das
@ 2023-03-09 9:58 ` Geert Uytterhoeven
2023-03-09 10:17 ` Krzysztof Kozlowski
2023-03-09 10:30 ` Biju Das
1 sibling, 2 replies; 26+ messages in thread
From: Geert Uytterhoeven @ 2023-03-09 9:58 UTC (permalink / raw)
To: Biju Das
Cc: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Geert Uytterhoeven,
linux-renesas-soc, linux-clk, devicetree, Fabrizio Castro
Hi Biju,
On Thu, Mar 9, 2023 at 10:44 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 09/03/2023 10:18, Biju Das wrote:
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> On 09/03/2023 08:57, Biju Das wrote:
> >>>>> It is clk generator HW specific. Clk generator is vital component
> >>>>> which provides clocks to the system.
> >>>>
> >>>> Every clock controller is vital...
> >>>>
> >>>>> We are providing some hardware feature which is exposed as dt
> >>>>> properties.
> >>>>>
> >>>>> Like clock output is fixed rate clock or dynamic rate clock/
> >>>>
> >>>> OK, I wait then for proper description which will explain and justify
> >> this.
> >>>
> >>> Here it is, Please let me know is it ok?
> >>>
> >>> renesas,output-clock-fixed-rate-mode:
> >>> type: boolean
> >>> description:
> >>> In output clock fixed rate mode, the output clock frequency is
> >> always
> >>> fixed and the hardware will use the values from the OTP or full
> >> register
> >>> map initialized during boot.
> >>> If not given, the output clock rate is not fixed.
> >>> maxItems: 6
> >>
> >> boolean is scalar, not array, so no maxItems. If the frequency is taken from
> >> OTP or register map, why they cannot also provide information the clock is
> >> fixed?
> >
> > OK, I will make an array property instead. From HW perspective each clock output from the
> > Clock generator is controllable ie, fixed rate or dynamic rate.
> >
> > If all the output clocks are fixed rate one, then frequency is taken from OTP or
> > register map. But if any one clock output generates dynamic rate, then it uses
> > dynamic settings.
>
> Second try, same question, let me know if it is not clear:
>
> "why they cannot also provide information the clock is fixed?"
What is the actual use case?
My understanding is:
1. If the OTP is programmed, the clock generator will be configured
from the OTP on power-on,
2. The clock generator can be (re)configured from software.
a. If the OTP is programmed, this is not needed,
b. For critical clocks, you may want to prevent this.
Also, AFAIUI, "fixed frequency" or "dynamic frequency" is a policy,
and purely software? Or are there OTP bits to enforce this?
Perhaps you need a per-output "do-not-change-frequency" flag,
probably with a generic name, in the spirit of "regulator-always-on"
for regulators?
Now, if all the output clocks are fixed rate, you might want to describe
this in DTS using a set of fixed{,-factor-}-clocks?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 9:53 ` Biju Das
@ 2023-03-09 10:15 ` Krzysztof Kozlowski
2023-03-09 11:18 ` Biju Das
0 siblings, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-09 10:15 UTC (permalink / raw)
To: Biju Das, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
On 09/03/2023 10:53, Biju Das wrote:
>
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Thursday, March 9, 2023 9:44 AM
>> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
>> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob Herring
>> <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; linux-renesas-
>> soc@vger.kernel.org; linux-clk@vger.kernel.org; devicetree@vger.kernel.org;
>> Fabrizio Castro <fabrizio.castro.jz@renesas.com>
>> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
>> generator bindings
>>
>> On 09/03/2023 10:18, Biju Das wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Sent: Thursday, March 9, 2023 9:14 AM
>>>> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
>>>> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob
>>>> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>>>> <krzysztof.kozlowski+dt@linaro.org>
>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; linux-renesas-
>>>> soc@vger.kernel.org; linux-clk@vger.kernel.org;
>>>> devicetree@vger.kernel.org; Fabrizio Castro
>>>> <fabrizio.castro.jz@renesas.com>
>>>> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3
>>>> clock generator bindings
>>>>
>>>> On 09/03/2023 08:57, Biju Das wrote:
>>>>>>> It is clk generator HW specific. Clk generator is vital component
>>>>>>> which provides clocks to the system.
>>>>>>
>>>>>> Every clock controller is vital...
>>>>>>
>>>>>>> We are providing some hardware feature which is exposed as dt
>>>>>>> properties.
>>>>>>>
>>>>>>> Like clock output is fixed rate clock or dynamic rate clock/
>>>>>>
>>>>>> OK, I wait then for proper description which will explain and
>>>>>> justify
>>>> this.
>>>>>
>>>>> Here it is, Please let me know is it ok?
>>>>>
>>>>> renesas,output-clock-fixed-rate-mode:
>>>>> type: boolean
>>>>> description:
>>>>> In output clock fixed rate mode, the output clock frequency is
>>>> always
>>>>> fixed and the hardware will use the values from the OTP or
>>>>> full
>>>> register
>>>>> map initialized during boot.
>>>>> If not given, the output clock rate is not fixed.
>>>>> maxItems: 6
>>>>
>>>> boolean is scalar, not array, so no maxItems. If the frequency is
>>>> taken from OTP or register map, why they cannot also provide
>>>> information the clock is fixed?
>>>
>>> OK, I will make an array property instead. From HW perspective each
>>> clock output from the Clock generator is controllable ie, fixed rate or
>> dynamic rate.
>>>
>>> If all the output clocks are fixed rate one, then frequency is taken
>>> from OTP or register map. But if any one clock output generates
>>> dynamic rate, then it uses dynamic settings.
>>
>> Second try, same question, let me know if it is not clear:
>>
>> "why they cannot also provide information the clock is fixed?"
>
> This information we are providing through dt.
No, you are not. We just discuss it. If we do not agree, you are not
going to provide information through DT.
>
> It is a complex clock generator which provides 6 HW clock outputs.
> The 6 HW clock outputs can be individually controllable to generate
> Either fixed frequency or dynamic frequency.
Ah, indeed. 6 clock outputs prohibits configuring this from OTP. If only
there were 5 outputs then it would be possible...
>
> Output clk1 "diff2",
> Output clk2 "diff1",
> Output clk3 "se3",
> Output clk4 "se2",
> Output clk5 "se1",
> Output clk6 "ref"
>
> I want to make "Output clk4" from clock generator as dynamic frequency one
> And make other clock frequency from clock generator as fixed one.
>
> How do you describe this in dt? Please share your thoughts.
Read from OTP or registers.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 9:58 ` Geert Uytterhoeven
@ 2023-03-09 10:17 ` Krzysztof Kozlowski
2023-03-09 10:25 ` Geert Uytterhoeven
2023-03-09 10:30 ` Biju Das
1 sibling, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-09 10:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Biju Das
Cc: Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Geert Uytterhoeven, linux-renesas-soc,
linux-clk, devicetree, Fabrizio Castro
On 09/03/2023 10:58, Geert Uytterhoeven wrote:
>>>>> Here it is, Please let me know is it ok?
>>>>>
>>>>> renesas,output-clock-fixed-rate-mode:
>>>>> type: boolean
>>>>> description:
>>>>> In output clock fixed rate mode, the output clock frequency is
>>>> always
>>>>> fixed and the hardware will use the values from the OTP or full
>>>> register
>>>>> map initialized during boot.
>>>>> If not given, the output clock rate is not fixed.
>>>>> maxItems: 6
>>>>
>>>> boolean is scalar, not array, so no maxItems. If the frequency is taken from
>>>> OTP or register map, why they cannot also provide information the clock is
>>>> fixed?
>>>
>>> OK, I will make an array property instead. From HW perspective each clock output from the
>>> Clock generator is controllable ie, fixed rate or dynamic rate.
>>>
>>> If all the output clocks are fixed rate one, then frequency is taken from OTP or
>>> register map. But if any one clock output generates dynamic rate, then it uses
>>> dynamic settings.
>>
>> Second try, same question, let me know if it is not clear:
>>
>> "why they cannot also provide information the clock is fixed?"
>
> What is the actual use case?
> My understanding is:
> 1. If the OTP is programmed, the clock generator will be configured
> from the OTP on power-on,
> 2. The clock generator can be (re)configured from software.
> a. If the OTP is programmed, this is not needed,
> b. For critical clocks, you may want to prevent this.
>
> Also, AFAIUI, "fixed frequency" or "dynamic frequency" is a policy,
> and purely software? Or are there OTP bits to enforce this?
>
> Perhaps you need a per-output "do-not-change-frequency" flag,
> probably with a generic name, in the spirit of "regulator-always-on"
> for regulators?
>
> Now, if all the output clocks are fixed rate, you might want to describe
> this in DTS using a set of fixed{,-factor-}-clocks?
>
I would also argue that fixed frequency is actually also dynamic
frequency, just with a limit to one frequency...
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 10:17 ` Krzysztof Kozlowski
@ 2023-03-09 10:25 ` Geert Uytterhoeven
2023-03-09 15:15 ` Biju Das
0 siblings, 1 reply; 26+ messages in thread
From: Geert Uytterhoeven @ 2023-03-09 10:25 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Biju Das, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Geert Uytterhoeven, linux-renesas-soc,
linux-clk, devicetree, Fabrizio Castro
Hi Krzysztof,
On Thu, Mar 9, 2023 at 11:17 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 09/03/2023 10:58, Geert Uytterhoeven wrote:
> >>>>> Here it is, Please let me know is it ok?
> >>>>>
> >>>>> renesas,output-clock-fixed-rate-mode:
> >>>>> type: boolean
> >>>>> description:
> >>>>> In output clock fixed rate mode, the output clock frequency is
> >>>> always
> >>>>> fixed and the hardware will use the values from the OTP or full
> >>>> register
> >>>>> map initialized during boot.
> >>>>> If not given, the output clock rate is not fixed.
> >>>>> maxItems: 6
> >>>>
> >>>> boolean is scalar, not array, so no maxItems. If the frequency is taken from
> >>>> OTP or register map, why they cannot also provide information the clock is
> >>>> fixed?
> >>>
> >>> OK, I will make an array property instead. From HW perspective each clock output from the
> >>> Clock generator is controllable ie, fixed rate or dynamic rate.
> >>>
> >>> If all the output clocks are fixed rate one, then frequency is taken from OTP or
> >>> register map. But if any one clock output generates dynamic rate, then it uses
> >>> dynamic settings.
> >>
> >> Second try, same question, let me know if it is not clear:
> >>
> >> "why they cannot also provide information the clock is fixed?"
> >
> > What is the actual use case?
> > My understanding is:
> > 1. If the OTP is programmed, the clock generator will be configured
> > from the OTP on power-on,
> > 2. The clock generator can be (re)configured from software.
> > a. If the OTP is programmed, this is not needed,
> > b. For critical clocks, you may want to prevent this.
> >
> > Also, AFAIUI, "fixed frequency" or "dynamic frequency" is a policy,
> > and purely software? Or are there OTP bits to enforce this?
> >
> > Perhaps you need a per-output "do-not-change-frequency" flag,
> > probably with a generic name, in the spirit of "regulator-always-on"
> > for regulators?
Actually we already have "assigned-clock{,-rate}s" properties for that.
> > Now, if all the output clocks are fixed rate, you might want to describe
> > this in DTS using a set of fixed{,-factor-}-clocks?
>
> I would also argue that fixed frequency is actually also dynamic
> frequency, just with a limit to one frequency...
Indeed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 9:58 ` Geert Uytterhoeven
2023-03-09 10:17 ` Krzysztof Kozlowski
@ 2023-03-09 10:30 ` Biju Das
1 sibling, 0 replies; 26+ messages in thread
From: Biju Das @ 2023-03-09 10:30 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Geert Uytterhoeven,
linux-renesas-soc, linux-clk, devicetree, Fabrizio Castro
Hi Geert Uytterhoeven,
Thanks for the feedback.
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
>
> Hi Biju,
>
> On Thu, Mar 9, 2023 at 10:44 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> > On 09/03/2023 10:18, Biju Das wrote:
> > >> -----Original Message-----
> > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> On
> > >> 09/03/2023 08:57, Biju Das wrote:
> > >>>>> It is clk generator HW specific. Clk generator is vital
> > >>>>> component which provides clocks to the system.
> > >>>>
> > >>>> Every clock controller is vital...
> > >>>>
> > >>>>> We are providing some hardware feature which is exposed as dt
> > >>>>> properties.
> > >>>>>
> > >>>>> Like clock output is fixed rate clock or dynamic rate clock/
> > >>>>
> > >>>> OK, I wait then for proper description which will explain and
> > >>>> justify
> > >> this.
> > >>>
> > >>> Here it is, Please let me know is it ok?
> > >>>
> > >>> renesas,output-clock-fixed-rate-mode:
> > >>> type: boolean
> > >>> description:
> > >>> In output clock fixed rate mode, the output clock frequency
> > >>> is
> > >> always
> > >>> fixed and the hardware will use the values from the OTP or
> > >>> full
> > >> register
> > >>> map initialized during boot.
> > >>> If not given, the output clock rate is not fixed.
> > >>> maxItems: 6
> > >>
> > >> boolean is scalar, not array, so no maxItems. If the frequency is
> > >> taken from OTP or register map, why they cannot also provide
> > >> information the clock is fixed?
> > >
> > > OK, I will make an array property instead. From HW perspective each
> > > clock output from the Clock generator is controllable ie, fixed rate or
> dynamic rate.
> > >
> > > If all the output clocks are fixed rate one, then frequency is taken
> > > from OTP or register map. But if any one clock output generates
> > > dynamic rate, then it uses dynamic settings.
> >
> > Second try, same question, let me know if it is not clear:
> >
> > "why they cannot also provide information the clock is fixed?"
>
> What is the actual use case?
> My understanding is:
> 1. If the OTP is programmed, the clock generator will be configured
> from the OTP on power-on,
Correct.
> 2. The clock generator can be (re)configured from software.
> a. If the OTP is programmed, this is not needed,
Yes, But we miss some HW functionality.
Eg:
On RZ/G2L SMARC EVK, By default audio mclk is connected to
11.2896 MHz clk (se2 output from clock generator) which is multiple of 44.1KHz.
and this clock is a non-critical clock.
48Khz playback/record is not possible with Audio codec, if we just use the
value from OTP.
But by changing parent of "se2 clock", it is possible to achieve
48 KHz playback.
> b. For critical clocks, you may want to prevent this.
For caseb, Critical clocks we won't change its registers.
The reconfiguration is only for non-critical clocks.
> Also, AFAIUI, "fixed frequency" or "dynamic frequency" is a policy, and
> purely software? Or are there OTP bits to enforce this?
Nothing OTP bits related.
>
> Perhaps you need a per-output "do-not-change-frequency" flag, probably with
> a generic name, in the spirit of "regulator-always-on"
> for regulators?
Yes "do-not-change-frequency" flag for per-output is sensible one.
> Now, if all the output clocks are fixed rate, you might want to describe
> this in DTS using a set of fixed{,-factor-}-clocks?
On Ideal case, all the output clocks are fixed rate and use the value from OTP.
But cases like, non critical clocks we should be able to
change frequency of that particular clock output.
In audio playback case, it is just 1 bit for changing the parent,
After the initial reconfiguration.
Cheers,
Biju
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 10:15 ` Krzysztof Kozlowski
@ 2023-03-09 11:18 ` Biju Das
0 siblings, 0 replies; 26+ messages in thread
From: Biju Das @ 2023-03-09 11:18 UTC (permalink / raw)
To: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski
Cc: Geert Uytterhoeven, linux-renesas-soc, linux-clk, devicetree,
Fabrizio Castro
Hi Krzysztof Kozlowski,
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
>
> On 09/03/2023 10:53, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Thursday, March 9, 2023 9:44 AM
> >> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
> >> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob
> >> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@linaro.org>
> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; linux-renesas-
> >> soc@vger.kernel.org; linux-clk@vger.kernel.org;
> >> devicetree@vger.kernel.org; Fabrizio Castro
> >> <fabrizio.castro.jz@renesas.com>
> >> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3
> >> clock generator bindings
> >>
> >> On 09/03/2023 10:18, Biju Das wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>>> Sent: Thursday, March 9, 2023 9:14 AM
> >>>> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
> >>>> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob
> >>>> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> >>>> <krzysztof.kozlowski+dt@linaro.org>
> >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; linux-renesas-
> >>>> soc@vger.kernel.org; linux-clk@vger.kernel.org;
> >>>> devicetree@vger.kernel.org; Fabrizio Castro
> >>>> <fabrizio.castro.jz@renesas.com>
> >>>> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3
> >>>> clock generator bindings
> >>>>
> >>>> On 09/03/2023 08:57, Biju Das wrote:
> >>>>>>> It is clk generator HW specific. Clk generator is vital
> >>>>>>> component which provides clocks to the system.
> >>>>>>
> >>>>>> Every clock controller is vital...
> >>>>>>
> >>>>>>> We are providing some hardware feature which is exposed as dt
> >>>>>>> properties.
> >>>>>>>
> >>>>>>> Like clock output is fixed rate clock or dynamic rate clock/
> >>>>>>
> >>>>>> OK, I wait then for proper description which will explain and
> >>>>>> justify
> >>>> this.
> >>>>>
> >>>>> Here it is, Please let me know is it ok?
> >>>>>
> >>>>> renesas,output-clock-fixed-rate-mode:
> >>>>> type: boolean
> >>>>> description:
> >>>>> In output clock fixed rate mode, the output clock frequency
> >>>>> is
> >>>> always
> >>>>> fixed and the hardware will use the values from the OTP or
> >>>>> full
> >>>> register
> >>>>> map initialized during boot.
> >>>>> If not given, the output clock rate is not fixed.
> >>>>> maxItems: 6
> >>>>
> >>>> boolean is scalar, not array, so no maxItems. If the frequency is
> >>>> taken from OTP or register map, why they cannot also provide
> >>>> information the clock is fixed?
> >>>
> >>> OK, I will make an array property instead. From HW perspective each
> >>> clock output from the Clock generator is controllable ie, fixed rate
> >>> or
> >> dynamic rate.
> >>>
> >>> If all the output clocks are fixed rate one, then frequency is taken
> >>> from OTP or register map. But if any one clock output generates
> >>> dynamic rate, then it uses dynamic settings.
> >>
> >> Second try, same question, let me know if it is not clear:
> >>
> >> "why they cannot also provide information the clock is fixed?"
> >
> > This information we are providing through dt.
>
> No, you are not. We just discuss it. If we do not agree, you are not going
> to provide information through DT.
>
> >
> > It is a complex clock generator which provides 6 HW clock outputs.
> > The 6 HW clock outputs can be individually controllable to generate
> > Either fixed frequency or dynamic frequency.
>
> Ah, indeed. 6 clock outputs prohibits configuring this from OTP. If only
> there were 5 outputs then it would be possible...
>
> >
> > Output clk1 "diff2",
> > Output clk2 "diff1",
> > Output clk3 "se3",
> > Output clk4 "se2",
> > Output clk5 "se1",
> > Output clk6 "ref"
> >
> > I want to make "Output clk4" from clock generator as dynamic frequency
> > one And make other clock frequency from clock generator as fixed one.
> >
> > How do you describe this in dt? Please share your thoughts.
>
> Read from OTP or registers.
Looks no other option. So will drop this property and match the one from OTP with full regmap.
Cheers,
Biju
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 2/3] drivers: clk: Add support for versa3 clock driver
2023-03-06 20:22 ` Stephen Boyd
@ 2023-03-09 12:25 ` Biju Das
0 siblings, 0 replies; 26+ messages in thread
From: Biju Das @ 2023-03-09 12:25 UTC (permalink / raw)
To: Stephen Boyd, Michael Turquette
Cc: linux-clk, Geert Uytterhoeven, Fabrizio Castro, linux-renesas-soc
Hi Stephen Boyd,
Thanks for the feedback.
> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Monday, March 6, 2023 8:22 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
> <mturquette@baylibre.com>
> Cc: Biju Das <biju.das.jz@bp.renesas.com>; linux-clk@vger.kernel.org; Geert
> Uytterhoeven <geert+renesas@glider.be>; Fabrizio Castro
> <fabrizio.castro.jz@renesas.com>; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH RFC 2/3] drivers: clk: Add support for versa3 clock
> driver
>
> Quoting Biju Das (2023-02-20 05:13:06)
> > diff --git a/drivers/clk/clk-versaclock3.c
> > b/drivers/clk/clk-versaclock3.c new file mode 100644 index
> > 000000000000..561cfad8a243
> > --- /dev/null
> > +++ b/drivers/clk/clk-versaclock3.c
> > @@ -0,0 +1,1134 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for Renesas Versaclock 3
> > + *
> > + * Copyright (C) 2021 Renesas Electronics Corp.
> > + */
> > +
> > +#include <linux/clk.h>
>
> Please remove this include after moving to 'struct clk_parent_data'.
Agreed.
>
> > +#include <linux/clk-provider.h>
> > +#include <linux/i2c.h>
> > +#include <linux/limits.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +#define NUM_CONFIG_REGISTERS 37
> > +
> [...]
> > +
> > +static void vc3_clk_flags_parse_dt(struct device *dev, u32 *crt_clks)
> > +{
> > + struct device_node *np = dev->of_node;
> > + struct property *prop;
> > + const __be32 *p;
> > + u32 i = 0;
> > + u32 val;
> > +
> > + of_property_for_each_u32(np, "renesas,clock-flags", prop, p, val)
> {
> > + if (i >= ARRAY_SIZE(clk_out_data))
> > + break;
> > + *crt_clks++ = val;
> > + i++;
> > + }
> > +}
> > +
> > +static void vc3_fill_init_data(struct clk_init_data *init,
> > + const struct vc3_clk_data *mux,
> > + const struct clk_ops *ops,
> > + u32 flags, int n,
> > + const char **pll_parent_names,
> > + const char **clkin_name) {
> > + unsigned int i;
> > +
> > + init->name = mux->name;
> > + init->ops = ops;
> > + init->flags = CLK_SET_RATE_PARENT;
> > + init->num_parents = n;
> > + for (i = 0; i < n; i++) {
> > + if (!mux->parent_names[i])
> > + pll_parent_names[i] = clkin_name[0];
> > + else
> > + pll_parent_names[i] = mux->parent_names[i];
>
> Instead of string names please use clk_hw pointers or 'struct
> clk_parent_data'.
OK.
>
> > + }
> > +
> > + init->parent_names = pll_parent_names; }
> > +
> > +static int vc3_clk_register(struct device *dev, struct vc3_driver_data
> *vc3,
> > + struct vc3_hw_data *data, const void
> *clk_data,
> > + struct clk_init_data *init, int n)
>
> const init pointer?
OK.
>
> > +{
> > + data->hw.init = init;
> > + data->num = n;
> > + data->vc3 = vc3;
> > + data->data = clk_data;
> > +
> > + return devm_clk_hw_register(dev, &data->hw); }
> > +
> > +static int vc3_probe(struct i2c_client *client) {
> > + struct device *dev = &client->dev;
> > + u8 settings[NUM_CONFIG_REGISTERS];
> > + const char *pll_parent_names[3];
> > + struct vc3_driver_data *vc3;
> > + const char *clkin_name[1];
> > + struct clk_init_data init;
> > + u32 crit_clks[6] = {};
> > + struct clk *clk;
> > + int ret, i;
> > +
> > + vc3 = devm_kzalloc(dev, sizeof(*vc3), GFP_KERNEL);
> > + if (!vc3)
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(client, vc3);
> > + vc3->client = client;
> > +
> > + vc3->regmap = devm_regmap_init_i2c(client, &vc3_regmap_config);
> > + if (IS_ERR(vc3->regmap))
> > + return dev_err_probe(dev, PTR_ERR(vc3->regmap),
> > + "failed to allocate register
> > + map\n");
> > +
> > + ret = of_property_read_u8_array(dev->of_node, "renesas,settings",
> > + settings, ARRAY_SIZE(settings));
> > + if (!ret) {
> > + /*
> > + * A raw settings array was specified in the DT. Write the
> > + * settings to the device immediately.
> > + */
> > + for (i = 0; i < NUM_CONFIG_REGISTERS; i++) {
> > + ret = regmap_write(vc3->regmap, i, settings[i]);
> > + if (ret) {
> > + dev_err(dev, "error writing to chip
> (%i)\n", ret);
> > + return ret;
> > + }
> > + }
> > + } else if (ret == -EOVERFLOW) {
> > + dev_err(&client->dev, "EOVERFLOW reg settings. ARRAY_SIZE:
> %zu",
> > + ARRAY_SIZE(settings));
> > + return ret;
> > + }
> > +
> > + /* Register clock ref */
> > + memset(&init, 0, sizeof(init));
> > +
> > + clk = devm_clk_get(dev, "x1");
> > + if (PTR_ERR(clk) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
> > +
> > + if (!IS_ERR(clk)) {
> > + clkin_name[0] = __clk_get_name(clk);
> > + } else {
> > + clk = devm_clk_get(dev, "clkin");
> > + if (PTR_ERR(clk) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
> > +
> > + if (!IS_ERR(clk))
> > + clkin_name[0] = __clk_get_name(clk);
> > + }
>
> Please use 'struct clk_parent_data' instead of clk consumer APIs.
OK.
>
> > +
> > + if (IS_ERR_OR_NULL(clk))
> > + return dev_err_probe(&client->dev, -EINVAL, "no input
> > + clk\n");
> > +
> > + /* Register pfd muxes */
> > + for (i = 0; i < ARRAY_SIZE(pfd_mux_data); i++) {
> > + vc3_fill_init_data(&init, &pfd_mux_data[i],
> &vc3_pfd_mux_ops,
> > + CLK_SET_RATE_PARENT, 2,
> pll_parent_names,
> > + clkin_name);
> > + ret = vc3_clk_register(dev, vc3, &vc3->clk_pfd_mux[i],
> > + &pfd_mux_data[i], &init, i);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "%s failed\n",
> init.name);
> > + }
> > +
> > + /* Register pfd's */
> > + for (i = 0; i < ARRAY_SIZE(pfd_data); i++) {
> > + if (i == VC3_PFD1)
> > + pll_parent_names[0] = clkin_name[0];
> > + else
> > + pll_parent_names[0] = pfd_mux_data[i -
> > + 1].name;
> > +
> > + init.name = pfd_data[i].name;
> > + init.ops = &vc3_pfd_ops;
> > + init.flags = CLK_SET_RATE_PARENT;
> > + init.num_parents = 1;
> > + init.parent_names = pll_parent_names;
> > +
> > + ret = vc3_clk_register(dev, vc3, &vc3->clk_pfd[i],
> > + &pfd_data[i], &init, i);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "%s failed\n",
> init.name);
> > + }
> > +
> > + /* Register pll's */
> > + for (i = 0; i < ARRAY_SIZE(pll_data); i++) {
> > + pll_parent_names[0] = pfd_data[i].name;
> > + init.name = pll_data[i].name;
> > + init.ops = &vc3_pll_ops;
> > + init.flags = CLK_SET_RATE_PARENT;
> > + init.num_parents = 1;
> > + init.parent_names = pll_parent_names;
> > +
> > + ret = vc3_clk_register(dev, vc3, &vc3->clk_pll[i],
> > + &pll_data[i], &init, i);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "%s failed\n",
> init.name);
> > + }
> > +
> > + /* Register divider muxes */
> > + for (i = 0; i < ARRAY_SIZE(div_mux_data); i++) {
> > + vc3_fill_init_data(&init, &div_mux_data[i],
> &vc3_div_mux_ops,
> > + CLK_SET_RATE_PARENT, 2,
> pll_parent_names,
> > + clkin_name);
> > + ret = vc3_clk_register(dev, vc3, &vc3->clk_div_mux[i],
> > + &div_mux_data[i], &init, i);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "%s failed\n",
> init.name);
> > + }
> > +
> > + vc3_divider_type_parse_dt(dev, vc3);
> > + /* Register dividers */
> > + for (i = 0; i < ARRAY_SIZE(div_data); i++) {
> > + switch (i) {
> > + case VC3_DIV1:
> > + pll_parent_names[0] =
> div_mux_data[VC3_DIV1_MUX].name;
> > + break;
> > + case VC3_DIV2:
> > + pll_parent_names[0] = pll_data[VC3_PLL1].name;
> > + break;
> > + case VC3_DIV3:
> > + pll_parent_names[0] =
> div_mux_data[VC3_DIV3_MUX].name;
> > + break;
> > + case VC3_DIV4:
> > + pll_parent_names[0] =
> div_mux_data[VC3_DIV4_MUX].name;
> > + break;
> > + case VC3_DIV5:
> > + pll_parent_names[0] = pll_data[VC3_PLL3].name;
> > + break;
> > + }
> > +
> > + init.name = div_data[i].name;
> > + init.ops = &vc3_div_ops;
> > + init.flags = CLK_SET_RATE_PARENT;
> > + init.num_parents = 1;
> > + init.parent_names = pll_parent_names;
> > +
> > + ret = vc3_clk_register(dev, vc3, &vc3->clk_div[i],
> > + &div_data[i], &init, i);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "%s failed\n",
> init.name);
> > + }
> > +
> > + /* Register clk muxes */
> > + for (i = 0; i < ARRAY_SIZE(clk_mux_data); i++) {
> > + vc3_fill_init_data(&init, &clk_mux_data[i],
> &vc3_clk_mux_ops,
> > + CLK_SET_RATE_PARENT, 2,
> pll_parent_names,
> > + clkin_name);
> > + ret = vc3_clk_register(dev, vc3, &vc3->clk_mux[i],
> > + &clk_mux_data[i], &init, i);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "%s failed\n",
> init.name);
> > + }
> > +
> > + /* Register clk outputs */
> > + vc3_clk_flags_parse_dt(dev, crit_clks);
> > + for (i = 0; i < ARRAY_SIZE(clk_out_data); i++) {
> > + vc3_fill_init_data(&init, &clk_out_data[i],
> &vc3_clk_out_ops,
> > + crit_clks[i], 1, pll_parent_names,
> > + clkin_name);
> > + ret = vc3_clk_register(dev, vc3, &vc3->clk_out[i],
> > + &clk_out_data[i], &init, i);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "%s failed\n",
> init.name);
> > + }
> > +
> > + ret = of_clk_add_hw_provider(client->dev.of_node,
> > + vc3_of_clk_get, vc3);
>
> Can you use devm?
OK.
>
> > + if (ret)
> > + return dev_err_probe(dev, ret, "unable to add clk
> > + provider\n");
> > +
> > + return ret;
> > +}
> > +
> > +static void vc3_remove(struct i2c_client *client) {
> > + of_clk_del_provider(client->dev.of_node);
> > +}
>
> Using devm means this whole function can be removed.
OK.
Will address this in next version.
Cheers,
Biju
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings
2023-03-09 10:25 ` Geert Uytterhoeven
@ 2023-03-09 15:15 ` Biju Das
0 siblings, 0 replies; 26+ messages in thread
From: Biju Das @ 2023-03-09 15:15 UTC (permalink / raw)
To: Geert Uytterhoeven, Krzysztof Kozlowski
Cc: Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Geert Uytterhoeven, linux-renesas-soc,
linux-clk, devicetree, Fabrizio Castro
Hi Geert Uytterhoeven,
Thanks for the feedback.
> Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock
> generator bindings
>
> Hi Krzysztof,
>
> On Thu, Mar 9, 2023 at 11:17 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> > On 09/03/2023 10:58, Geert Uytterhoeven wrote:
> > >>>>> Here it is, Please let me know is it ok?
> > >>>>>
> > >>>>> renesas,output-clock-fixed-rate-mode:
> > >>>>> type: boolean
> > >>>>> description:
> > >>>>> In output clock fixed rate mode, the output clock
> > >>>>> frequency is
> > >>>> always
> > >>>>> fixed and the hardware will use the values from the OTP or
> > >>>>> full
> > >>>> register
> > >>>>> map initialized during boot.
> > >>>>> If not given, the output clock rate is not fixed.
> > >>>>> maxItems: 6
> > >>>>
> > >>>> boolean is scalar, not array, so no maxItems. If the frequency is
> > >>>> taken from OTP or register map, why they cannot also provide
> > >>>> information the clock is fixed?
> > >>>
> > >>> OK, I will make an array property instead. From HW perspective
> > >>> each clock output from the Clock generator is controllable ie, fixed
> rate or dynamic rate.
> > >>>
> > >>> If all the output clocks are fixed rate one, then frequency is
> > >>> taken from OTP or register map. But if any one clock output
> > >>> generates dynamic rate, then it uses dynamic settings.
> > >>
> > >> Second try, same question, let me know if it is not clear:
> > >>
> > >> "why they cannot also provide information the clock is fixed?"
> > >
> > > What is the actual use case?
> > > My understanding is:
> > > 1. If the OTP is programmed, the clock generator will be configured
> > > from the OTP on power-on,
> > > 2. The clock generator can be (re)configured from software.
> > > a. If the OTP is programmed, this is not needed,
> > > b. For critical clocks, you may want to prevent this.
> > >
> > > Also, AFAIUI, "fixed frequency" or "dynamic frequency" is a policy,
> > > and purely software? Or are there OTP bits to enforce this?
> > >
> > > Perhaps you need a per-output "do-not-change-frequency" flag,
> > > probably with a generic name, in the spirit of "regulator-always-on"
> > > for regulators?
>
> Actually we already have "assigned-clock{,-rate}s" properties for that.
Actually it is a good pointer. I can make use of this property.
Cheers,
Biju
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2023-03-09 15:16 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-20 13:13 [PATCH RFC 0/3] Add Versa3 clock generator support Biju Das
2023-02-20 13:13 ` [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings Biju Das
2023-02-22 9:34 ` Krzysztof Kozlowski
2023-03-08 14:39 ` Biju Das
2023-03-08 14:50 ` Geert Uytterhoeven
2023-03-08 14:57 ` Biju Das
2023-03-08 18:47 ` Krzysztof Kozlowski
2023-03-08 18:55 ` Biju Das
2023-03-08 19:17 ` Krzysztof Kozlowski
2023-03-09 7:57 ` Biju Das
2023-03-09 9:13 ` Krzysztof Kozlowski
2023-03-09 9:18 ` Biju Das
2023-03-09 9:44 ` Krzysztof Kozlowski
2023-03-09 9:53 ` Biju Das
2023-03-09 10:15 ` Krzysztof Kozlowski
2023-03-09 11:18 ` Biju Das
2023-03-09 9:58 ` Geert Uytterhoeven
2023-03-09 10:17 ` Krzysztof Kozlowski
2023-03-09 10:25 ` Geert Uytterhoeven
2023-03-09 15:15 ` Biju Das
2023-03-09 10:30 ` Biju Das
2023-02-20 13:13 ` [PATCH RFC 2/3] drivers: clk: Add support for versa3 clock driver Biju Das
2023-03-06 20:22 ` Stephen Boyd
2023-03-09 12:25 ` Biju Das
2023-02-20 13:13 ` [PATCH RFC 3/3] arm64: dts: renesas: rzg2l-smarc: Use versa3 clk for audio mclk Biju Das
2023-02-20 13:18 ` [PATCH RFC 0/3] Add Versa3 clock generator support Biju Das
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).