[PATCH 0/6] Add PolarFire SoC Fabric Clock Conditioning Circuitry Support

[PATCH 0/6] Add PolarFire SoC Fabric Clock Conditioning Circuitry Support

[Conor Dooley]

Hey all,

PolarFire SoC has 4 clock source blocks, each with 2 PLLs and 2 DLLs,
in the corners of the FPGA fabric. Add bindings, a driver supporting
the PLLs and the requisite changes to the devicetrees for PolarFire
SoC based boards. These clocks were already in use, but which clock
specifically was chosen was decided by the synthesis tool. In our
end-of-September release of our FPGA reference design, constraints will
be added to force the synthesis tool to pick the "north west" CCC,
making it possible to read the configuration from the CCC's registers.

I am mainly looking for feedback on the dt-bindings on this version,
so that if something dt-abi related needs to change it can be done in
advance.

There are no maintainers changes in this series, but they are required
due to the binding rename. I am waiting for some changes queued in the
soc tree before rebasing on a later -rc before including that patch.

Thanks,
Conor.

Conor Dooley (6):
  dt-bindings: clk: rename mpfs-clkcfg binding
  dt-bindings: clk: document PolarFire SoC fabric clocks
  dt-bindings: clk: add PolarFire SoC fabric clock ids
  clk: microchip: add PolarFire SoC fabric clock support
  dt-bindings: riscv: microchip: document icicle reference design
  riscv: dts: microchip: add the mpfs' fabric clock control

 .../bindings/clock/microchip,mpfs-ccc.yaml    |  80 +++++
 ...p,mpfs.yaml => microchip,mpfs-clkcfg.yaml} |   2 +-
 .../devicetree/bindings/riscv/microchip.yaml  |   1 +
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |  27 +-
 .../boot/dts/microchip/mpfs-icicle-kit.dts    |   4 +
 .../dts/microchip/mpfs-polarberry-fabric.dtsi |   5 +
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |  34 +-
 drivers/clk/microchip/Makefile                |   1 +
 drivers/clk/microchip/clk-mpfs-ccc.c          | 294 ++++++++++++++++++
 .../dt-bindings/clock/microchip,mpfs-clock.h  |  23 ++
 10 files changed, 458 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/microchip,mpfs-ccc.yaml
 rename Documentation/devicetree/bindings/clock/{microchip,mpfs.yaml => microchip,mpfs-clkcfg.yaml} (96%)
 create mode 100644 drivers/clk/microchip/clk-mpfs-ccc.c


base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
-- 
2.36.1

[PATCH 1/6] dt-bindings: clk: rename mpfs-clkcfg binding

[Conor Dooley]

The filename for a binding is supposed to match the first compatible,
but the mpfs-clkcfg file did not follow this policy. Rename it to match
so that when other mpfs clock bindings are added things make more sense.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../clock/{microchip,mpfs.yaml => microchip,mpfs-clkcfg.yaml}   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename Documentation/devicetree/bindings/clock/{microchip,mpfs.yaml => microchip,mpfs-clkcfg.yaml} (96%)

diff --git a/Documentation/devicetree/bindings/clock/microchip,mpfs.yaml b/Documentation/devicetree/bindings/clock/microchip,mpfs-clkcfg.yaml
similarity index 96%
rename from Documentation/devicetree/bindings/clock/microchip,mpfs.yaml
rename to Documentation/devicetree/bindings/clock/microchip,mpfs-clkcfg.yaml
index 016a4f378b9b..212228734ffd 100644
--- a/Documentation/devicetree/bindings/clock/microchip,mpfs.yaml
+++ b/Documentation/devicetree/bindings/clock/microchip,mpfs-clkcfg.yaml
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/clock/microchip,mpfs.yaml#
+$id: http://devicetree.org/schemas/clock/microchip,mpfs-clkcfg.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: Microchip PolarFire Clock Control Module Binding
-- 
2.36.1

Re: [PATCH 1/6] dt-bindings: clk: rename mpfs-clkcfg binding

[Krzysztof Kozlowski]

On 19/08/2022 15:22, Conor Dooley wrote:
> The filename for a binding is supposed to match the first compatible,
> but the mpfs-clkcfg file did not follow this policy. Rename it to match
> so that when other mpfs clock bindings are added things make more sense.
> 


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

[PATCH 2/6] dt-bindings: clk: document PolarFire SoC fabric clocks

[Conor Dooley]

On PolarFire SoC there are 4 PLL/DLL blocks, located in each of the
ordinal corners of the chip, which our documentation refers to as
"Clock Conditioning Circuitry". PolarFire SoC is an FPGA, these are
highly configurable & many of the input clocks are optional.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/clock/microchip,mpfs-ccc.yaml    | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/microchip,mpfs-ccc.yaml

diff --git a/Documentation/devicetree/bindings/clock/microchip,mpfs-ccc.yaml b/Documentation/devicetree/bindings/clock/microchip,mpfs-ccc.yaml
new file mode 100644
index 000000000000..2e78aa15dbe0
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/microchip,mpfs-ccc.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/microchip,mpfs-ccc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip PolarFire SoC Fabric Clock Conditioning Circuitry
+
+maintainers:
+  - Conor Dooley <conor.dooley@microchip.com>
+
+description: |
+  Microchip PolarFire SoC has 4 Clock Conditioning Circuitry blocks. Each of
+  these blocks contains two PLLs and 2 DLLs & are located in the four corners of
+  the FPGA. For more information see "PolarFire SoC FPGA Clocking Resources" at:
+  https://onlinedocs.microchip.com/pr/GUID-8F0CC4C0-0317-4262-89CA-CE7773ED1931-en-US-1/index.html
+
+properties:
+  compatible:
+    const: microchip,mpfs-ccc
+
+  reg:
+    items:
+      - description: PLL0's control registers
+      - description: PLL1's control registers
+      - description: DLL0's control registers
+      - description: DLL1's control registers
+
+  clocks:
+    description:
+      The CCC PLL's have two input clocks. It is required that even if the input
+      clocks are identical that both are provided.
+    minItems: 2
+    items:
+      - description: PLL0's refclk0
+      - description: PLL0's refclk1
+      - description: PLL1's refclk0
+      - description: PLL1's refclk1
+      - description: DLL0's refclk
+      - description: DLL1's refclk
+
+  clock-names:
+    minItems: 2
+    items:
+      - const: pll0_ref0
+      - const: pll0_ref1
+      - const: pll1_ref0
+      - const: pll1_ref1
+      - const: dll0_ref
+      - const: dll1_ref
+
+  '#clock-cells':
+    const: 1
+    description: |
+      The clock consumer should specify the desired clock by having the clock
+      ID in its "clocks" phandle cell.
+      See include/dt-bindings/clock/microchip,mpfs-clock.h for the full list of
+      PolarFire clock IDs.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    ccc_nw: cccnwclk@38100000 {
+        compatible = "microchip,mpfs-ccc";
+        reg = <0x38010000 0x1000>, <0x38020000 0x1000>,
+              <0x39010000 0x1000>, <0x39020000 0x1000>;
+        #clock-cells = <1>;
+        clocks = <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>,
+                  <&refclk_ccc>, <&refclk_ccc>;
+        clock-names = "pll0_ref0", "pll0_ref1", "pll1_ref0", "pll1_ref1",
+                      "dll0_ref", "dll1_ref";
+    };
-- 
2.36.1

Re: [PATCH 2/6] dt-bindings: clk: document PolarFire SoC fabric clocks

[Krzysztof Kozlowski]

On 19/08/2022 15:22, Conor Dooley wrote:
> On PolarFire SoC there are 4 PLL/DLL blocks, located in each of the
> ordinal corners of the chip, which our documentation refers to as
> "Clock Conditioning Circuitry". PolarFire SoC is an FPGA, these are
> highly configurable & many of the input clocks are optional.
> 

Thank you for your patch. There is something to discuss/improve.

> +  '#clock-cells':
> +    const: 1
> +    description: |
> +      The clock consumer should specify the desired clock by having the clock
> +      ID in its "clocks" phandle cell.
> +      See include/dt-bindings/clock/microchip,mpfs-clock.h for the full list of
> +      PolarFire clock IDs.
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    ccc_nw: cccnwclk@38100000 {

Node names should be generic: clock-controller

https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


Best regards,
Krzysztof

Re: [PATCH 2/6] dt-bindings: clk: document PolarFire SoC fabric clocks

[Conor.Dooley]

On 19/08/2022 13:45, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 19/08/2022 15:22, Conor Dooley wrote:
>> On PolarFire SoC there are 4 PLL/DLL blocks, located in each of the
>> ordinal corners of the chip, which our documentation refers to as
>> "Clock Conditioning Circuitry". PolarFire SoC is an FPGA, these are
>> highly configurable & many of the input clocks are optional.
>>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +  '#clock-cells':
>> +    const: 1
>> +    description: |
>> +      The clock consumer should specify the desired clock by having the clock
>> +      ID in its "clocks" phandle cell.
>> +      See include/dt-bindings/clock/microchip,mpfs-clock.h for the full list of
>> +      PolarFire clock IDs.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - '#clock-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    ccc_nw: cccnwclk@38100000 {
> 
> Node names should be generic: clock-controller
> 
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Further, the label is not required in the example.
I'll fix this for v2, thanks.
Conor.

[PATCH 3/6] dt-bindings: clk: add PolarFire SoC fabric clock ids

[Conor Dooley]

Each Clock Conditioning Circuitry block contains 2 PLLs and 2 DLLs.
The PLLs have 4 outputs each and the DLLs 2. Add 16 new IDs covering
these clocks. For more information on the CCC hardware, see the
"PolarFire SoC FPGA Clocking Resources" document at the link below.

Link: https://onlinedocs.microchip.com/pr/GUID-8F0CC4C0-0317-4262-89CA-CE7773ED1931-en-US-1/index.html
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../dt-bindings/clock/microchip,mpfs-clock.h  | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/dt-bindings/clock/microchip,mpfs-clock.h b/include/dt-bindings/clock/microchip,mpfs-clock.h
index 4048669bf756..79775a5134ca 100644
--- a/include/dt-bindings/clock/microchip,mpfs-clock.h
+++ b/include/dt-bindings/clock/microchip,mpfs-clock.h
@@ -45,4 +45,27 @@
 #define CLK_RTCREF	33
 #define CLK_MSSPLL	34
 
+/* Clock Conditioning Circuitry Clock IDs */
+
+#define CLK_CCC_PLL0		0
+#define CLK_CCC_PLL1		1
+#define CLK_CCC_DLL0		2
+#define CLK_CCC_DLL1		3
+
+#define CLK_CCC_PLL0_OUT0	4
+#define CLK_CCC_PLL0_OUT1	5
+#define CLK_CCC_PLL0_OUT2	6
+#define CLK_CCC_PLL0_OUT3	7
+
+#define CLK_CCC_PLL1_OUT0	8
+#define CLK_CCC_PLL1_OUT1	9
+#define CLK_CCC_PLL1_OUT2	10
+#define CLK_CCC_PLL1_OUT3	11
+
+#define CLK_CCC_DLL0_OUT0	12
+#define CLK_CCC_DLL0_OUT1	13
+
+#define CLK_CCC_DLL1_OUT0	14
+#define CLK_CCC_DLL1_OUT1	15
+
 #endif	/* _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ */
-- 
2.36.1

Re: [PATCH 3/6] dt-bindings: clk: add PolarFire SoC fabric clock ids

[Krzysztof Kozlowski]

On 19/08/2022 15:22, Conor Dooley wrote:
> Each Clock Conditioning Circuitry block contains 2 PLLs and 2 DLLs.
> The PLLs have 4 outputs each and the DLLs 2. Add 16 new IDs covering
> these clocks. For more information on the CCC hardware, see the
> "PolarFire SoC FPGA Clocking Resources" document at the link below.
> 
> Link: https://onlinedocs.microchip.com/pr/GUID-8F0CC4C0-0317-4262-89CA-CE7773ED1931-en-US-1/index.html
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  .../dt-bindings/clock/microchip,mpfs-clock.h  | 23 +++++++++++++++++++

This could be also a separate header file for CCC.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

[PATCH 4/6] clk: microchip: add PolarFire SoC fabric clock support

[Conor Dooley]

Add a driver to support the PLLs in PolarFire SoC's Clock Conditioning
Circuitry, an instance of which is located in each ordinal corner of
the FPGA. Only get_rate() is supported as these clocks are intended to
be statically configured by the FPGA design. Currently, the DLLs are
not supported by this driver. For more information on the hardware, see
"PolarFire SoC FPGA Clocking Resources" in the link below.

Link: https://onlinedocs.microchip.com/pr/GUID-8F0CC4C0-0317-4262-89CA-CE7773ED1931-en-US-1/index.html
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/clk/microchip/Makefile       |   1 +
 drivers/clk/microchip/clk-mpfs-ccc.c | 294 +++++++++++++++++++++++++++
 2 files changed, 295 insertions(+)
 create mode 100644 drivers/clk/microchip/clk-mpfs-ccc.c

diff --git a/drivers/clk/microchip/Makefile b/drivers/clk/microchip/Makefile
index 5fa6dcf30a9a..13250e04e46c 100644
--- a/drivers/clk/microchip/Makefile
+++ b/drivers/clk/microchip/Makefile
@@ -2,3 +2,4 @@
 obj-$(CONFIG_COMMON_CLK_PIC32) += clk-core.o
 obj-$(CONFIG_PIC32MZDA) += clk-pic32mzda.o
 obj-$(CONFIG_MCHP_CLK_MPFS) += clk-mpfs.o
+obj-$(CONFIG_MCHP_CLK_MPFS) += clk-mpfs-ccc.o
diff --git a/drivers/clk/microchip/clk-mpfs-ccc.c b/drivers/clk/microchip/clk-mpfs-ccc.c
new file mode 100644
index 000000000000..d6a1777c428c
--- /dev/null
+++ b/drivers/clk/microchip/clk-mpfs-ccc.c
@@ -0,0 +1,294 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Author: Conor Dooley <conor.dooley@microchip.com>
+ *
+ * Copyright (C) 2022 Microchip Technology Inc. and its subsidiaries
+ */
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <dt-bindings/clock/microchip,mpfs-clock.h>
+
+/* address offset of control registers */
+#define MPFS_CCC_PLL_CR			0x04u
+#define MPFS_CCC_REF_CR			0x08u
+#define MPFS_CCC_SSCG_2_CR		0x2Cu
+#define MPFS_CCC_POSTDIV01_CR		0x10u
+#define MPFS_CCC_POSTDIV23_CR		0x14u
+
+#define MPFS_CCC_FBDIV_SHIFT		0x00u
+#define MPFS_CCC_FBDIV_WIDTH		0x0Cu
+#define MPFS_CCC_POSTDIV0_SHIFT		0x08u
+#define MPFS_CCC_POSTDIV1_SHIFT		0x18u
+#define MPFS_CCC_POSTDIV2_SHIFT		MPFS_CCC_POSTDIV0_SHIFT
+#define MPFS_CCC_POSTDIV3_SHIFT		MPFS_CCC_POSTDIV1_SHIFT
+#define MPFS_CCC_POSTDIV_WIDTH		0x06u
+#define MPFS_CCC_REFCLK_SEL		BIT(6)
+#define MPFS_CCC_REFDIV_SHIFT		0x08u
+#define MPFS_CCC_REFDIV_WIDTH		0x06u
+
+#define MPFS_CCC_FIXED_DIV		4
+#define MPFS_CCC_OUTPUTS_PER_PLL	4
+#define MPFS_CCC_REFS_PER_PLL		2
+
+struct mpfs_ccc_data {
+	void __iomem **pll_base;
+	struct device *dev;
+	const char *location;
+	struct clk_hw_onecell_data hw_data;
+};
+
+struct mpfs_ccc_pll_hw_clock {
+	void __iomem *base;
+	const char *name;
+	const struct clk_parent_data *parents;
+	unsigned int id;
+	u32 reg_offset;
+	u32 shift;
+	u32 width;
+	u32 flags;
+	struct clk_hw hw;
+	struct clk_init_data init;
+};
+
+#define to_mpfs_ccc_clk(_hw) container_of(_hw, struct mpfs_ccc_pll_hw_clock, hw)
+
+/*
+ * mpfs_ccc_lock prevents anything else from writing to a fabric ccc
+ * while a software locked register is being written.
+ */
+static DEFINE_SPINLOCK(mpfs_ccc_lock);
+
+static const struct clk_parent_data mpfs_ccc_pll0_refs[] = {
+	{ .fw_name = "pll0_ref0" },
+	{ .fw_name = "pll0_ref1" },
+};
+
+static const struct clk_parent_data mpfs_ccc_pll1_refs[] = {
+	{ .fw_name = "pll1_ref0" },
+	{ .fw_name = "pll1_ref1" },
+};
+
+static unsigned long mpfs_ccc_pll_recalc_rate(struct clk_hw *hw, unsigned long prate)
+{
+	struct mpfs_ccc_pll_hw_clock *ccc_hw = to_mpfs_ccc_clk(hw);
+	void __iomem *mult_addr = ccc_hw->base + ccc_hw->reg_offset;
+	void __iomem *ref_div_addr = ccc_hw->base + MPFS_CCC_REF_CR;
+	u32 mult, ref_div;
+
+	mult = readl_relaxed(mult_addr) >> MPFS_CCC_FBDIV_SHIFT;
+	mult &= clk_div_mask(MPFS_CCC_FBDIV_WIDTH);
+	ref_div = readl_relaxed(ref_div_addr) >> MPFS_CCC_REFDIV_SHIFT;
+	ref_div &= clk_div_mask(MPFS_CCC_REFDIV_WIDTH);
+
+	return prate * mult / (ref_div * MPFS_CCC_FIXED_DIV);
+}
+
+static u8 mpfs_ccc_pll_get_parent(struct clk_hw *hw)
+{
+	struct mpfs_ccc_pll_hw_clock *ccc_hw = to_mpfs_ccc_clk(hw);
+	void __iomem *pll_cr_addr = ccc_hw->base + MPFS_CCC_PLL_CR;
+
+	return !!(readl_relaxed(pll_cr_addr) & MPFS_CCC_REFCLK_SEL);
+}
+
+static const struct clk_ops mpfs_ccc_pll_ops = {
+	.recalc_rate = mpfs_ccc_pll_recalc_rate,
+	.get_parent = mpfs_ccc_pll_get_parent,
+};
+
+#define CLK_CCC_PLL(_id, _parents, _shift, _width, _flags, _offset) {		\
+	.id = _id,									\
+	.shift = _shift,								\
+	.width = _width,								\
+	.reg_offset = _offset,								\
+	.flags = _flags,								\
+	.parents = _parents,\
+}
+
+static struct mpfs_ccc_pll_hw_clock mpfs_ccc_pll_clks[] = {
+	CLK_CCC_PLL(CLK_CCC_PLL0, mpfs_ccc_pll0_refs, MPFS_CCC_FBDIV_SHIFT,
+		    MPFS_CCC_FBDIV_WIDTH, 0, MPFS_CCC_SSCG_2_CR),
+	CLK_CCC_PLL(CLK_CCC_PLL1, mpfs_ccc_pll1_refs, MPFS_CCC_FBDIV_SHIFT,
+		    MPFS_CCC_FBDIV_WIDTH, 0, MPFS_CCC_SSCG_2_CR),
+};
+
+struct mpfs_ccc_out_hw_clock {
+	struct clk_divider divider;
+	struct clk_init_data init;
+	unsigned int id;
+	u32 reg_offset;
+};
+
+#define CLK_CCC_OUT(_id, _shift, _width, _flags, _offset) {	\
+	.id = _id,						\
+	.divider.shift = _shift,				\
+	.divider.width = _width,				\
+	.reg_offset = _offset,					\
+	.divider.flags = _flags,				\
+	.divider.lock = &mpfs_ccc_lock,				\
+}
+
+static struct mpfs_ccc_out_hw_clock mpfs_ccc_pll0out_clks[] = {
+	CLK_CCC_OUT(CLK_CCC_PLL0_OUT0, MPFS_CCC_POSTDIV0_SHIFT, MPFS_CCC_POSTDIV_WIDTH,
+		    CLK_DIVIDER_ONE_BASED, MPFS_CCC_POSTDIV01_CR),
+	CLK_CCC_OUT(CLK_CCC_PLL0_OUT1, MPFS_CCC_POSTDIV1_SHIFT, MPFS_CCC_POSTDIV_WIDTH,
+		    CLK_DIVIDER_ONE_BASED, MPFS_CCC_POSTDIV01_CR),
+	CLK_CCC_OUT(CLK_CCC_PLL0_OUT2, MPFS_CCC_POSTDIV2_SHIFT, MPFS_CCC_POSTDIV_WIDTH,
+		    CLK_DIVIDER_ONE_BASED, MPFS_CCC_POSTDIV23_CR),
+	CLK_CCC_OUT(CLK_CCC_PLL0_OUT3, MPFS_CCC_POSTDIV3_SHIFT, MPFS_CCC_POSTDIV_WIDTH,
+		    CLK_DIVIDER_ONE_BASED, MPFS_CCC_POSTDIV23_CR),
+};
+
+static struct mpfs_ccc_out_hw_clock mpfs_ccc_pll1out_clks[] = {
+	CLK_CCC_OUT(CLK_CCC_PLL1_OUT0, MPFS_CCC_POSTDIV0_SHIFT, MPFS_CCC_POSTDIV_WIDTH, 0,
+		    MPFS_CCC_POSTDIV01_CR),
+	CLK_CCC_OUT(CLK_CCC_PLL1_OUT1, MPFS_CCC_POSTDIV1_SHIFT, MPFS_CCC_POSTDIV_WIDTH, 0,
+		    MPFS_CCC_POSTDIV01_CR),
+	CLK_CCC_OUT(CLK_CCC_PLL1_OUT2, MPFS_CCC_POSTDIV2_SHIFT, MPFS_CCC_POSTDIV_WIDTH, 0,
+		    MPFS_CCC_POSTDIV23_CR),
+	CLK_CCC_OUT(CLK_CCC_PLL1_OUT3, MPFS_CCC_POSTDIV3_SHIFT, MPFS_CCC_POSTDIV_WIDTH, 0,
+		    MPFS_CCC_POSTDIV23_CR),
+};
+
+static struct mpfs_ccc_out_hw_clock *mpfs_ccc_pllout_clks[] = {
+	mpfs_ccc_pll0out_clks, mpfs_ccc_pll1out_clks
+};
+
+static int mpfs_ccc_register_outputs(struct device *dev, struct mpfs_ccc_out_hw_clock *out_hws,
+				     unsigned int num_clks, struct mpfs_ccc_data *data,
+				     struct mpfs_ccc_pll_hw_clock *parent)
+{
+	int ret;
+
+	for (unsigned int i = 0; i < num_clks; i++) {
+		struct mpfs_ccc_out_hw_clock *out_hw = &out_hws[i];
+		char *name = devm_kzalloc(dev, 13, GFP_KERNEL);
+
+		snprintf(name, 19, "%s_out%u", parent->name, i);
+		out_hw->divider.hw.init = CLK_HW_INIT_HW(name, &parent->hw, &clk_divider_ops, 0);
+		out_hw->divider.reg = data->pll_base[i / MPFS_CCC_OUTPUTS_PER_PLL] +
+			out_hw->reg_offset;
+
+		ret = devm_clk_hw_register(dev, &out_hw->divider.hw);
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to register clock id: %d\n",
+					     out_hw->id);
+
+		data->hw_data.hws[out_hw->id] = &out_hw->divider.hw;
+	}
+
+	return 0;
+}
+
+#define CLK_HW_INIT_PARENTS_DATA_FIXED_SIZE(_name, _parents, _ops, _flags)	\
+	(&(struct clk_init_data) {						\
+		.flags		= _flags,					\
+		.name		= _name,					\
+		.parent_data	= _parents,					\
+		.num_parents	= MPFS_CCC_REFS_PER_PLL,			\
+		.ops		= _ops,						\
+	})
+
+static int mpfs_ccc_register_plls(struct device *dev, struct mpfs_ccc_pll_hw_clock *pll_hws,
+				  unsigned int num_clks, struct mpfs_ccc_data *data)
+{
+	int ret;
+
+	for (unsigned int i = 0; i < num_clks; i++) {
+		struct mpfs_ccc_pll_hw_clock *pll_hw = &pll_hws[i];
+		char *name = devm_kzalloc(dev, 8, GFP_KERNEL);
+
+		snprintf(name, 14, "%s_pll%u", dev->of_node->name, i);
+		pll_hw->name = (const char *)name;
+		pll_hw->base = data->pll_base[i];
+		pll_hw->hw.init = CLK_HW_INIT_PARENTS_DATA_FIXED_SIZE(pll_hw->name,
+								      pll_hw->parents,
+								      &mpfs_ccc_pll_ops, 0);
+
+		ret = devm_clk_hw_register(dev, &pll_hw->hw);
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to register ccc id: %d\n",
+					     pll_hw->id);
+
+		data->hw_data.hws[pll_hw->id] = &pll_hw->hw;
+
+		ret = mpfs_ccc_register_outputs(dev, mpfs_ccc_pllout_clks[i],
+						MPFS_CCC_OUTPUTS_PER_PLL, data, pll_hw);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int mpfs_ccc_probe(struct platform_device *pdev)
+{
+	struct mpfs_ccc_data *clk_data;
+	void __iomem *pll_base[ARRAY_SIZE(mpfs_ccc_pll_clks)];
+	unsigned int num_clks;
+	int ret;
+
+	num_clks = ARRAY_SIZE(mpfs_ccc_pll_clks) + ARRAY_SIZE(mpfs_ccc_pll0out_clks)
+		+ ARRAY_SIZE(mpfs_ccc_pll1out_clks);
+
+	clk_data = devm_kzalloc(&pdev->dev, struct_size(clk_data, hw_data.hws, num_clks),
+				GFP_KERNEL);
+	if (!clk_data)
+		return -ENOMEM;
+
+	pll_base[0] = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pll_base[0]))
+		return PTR_ERR(pll_base[0]);
+
+	pll_base[1] = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(pll_base[1]))
+		return PTR_ERR(pll_base[1]);
+
+	clk_data->pll_base = pll_base;
+
+	clk_data->dev = &pdev->dev;
+
+	ret = mpfs_ccc_register_plls(clk_data->dev, mpfs_ccc_pll_clks,
+				     ARRAY_SIZE(mpfs_ccc_pll_clks), clk_data);
+	if (ret)
+		return ret;
+
+	ret = devm_of_clk_add_hw_provider(clk_data->dev, of_clk_hw_onecell_get,
+					  &clk_data->hw_data);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+static const struct of_device_id mpfs_ccc_of_match_table[] = {
+	{ .compatible = "microchip,mpfs-ccc", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mpfs_ccc_of_match_table);
+
+static struct platform_driver mpfs_ccc_driver = {
+	.probe = mpfs_ccc_probe,
+	.driver	= {
+		.name = "microchip-mpfs-ccc",
+		.of_match_table = mpfs_ccc_of_match_table,
+	},
+};
+
+static int __init clk_ccc_init(void)
+{
+	return platform_driver_register(&mpfs_ccc_driver);
+}
+core_initcall(clk_ccc_init);
+
+static void __exit clk_ccc_exit(void)
+{
+	platform_driver_unregister(&mpfs_ccc_driver);
+}
+module_exit(clk_ccc_exit);
+
+MODULE_DESCRIPTION("Microchip PolarFire SoC Clock Conditioning Circuitry Driver");
+MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>");
+MODULE_LICENSE("GPL");
-- 
2.36.1

[PATCH 5/6] dt-bindings: riscv: microchip: document icicle reference design

[Conor Dooley]

Add a new compatible for the icicle kit reference design's 22.09
release, which made some changes to the memory map - including adding
the ability to read the CCCs via the system controller bus. Technically
that was always possible, but the specific CC chosen could vary per
run of the synthesis tool. Hopefully this is the last reference design
version impacting the memory map.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/devicetree/bindings/riscv/microchip.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/riscv/microchip.yaml b/Documentation/devicetree/bindings/riscv/microchip.yaml
index 1aa7336a9672..928ce4d4e087 100644
--- a/Documentation/devicetree/bindings/riscv/microchip.yaml
+++ b/Documentation/devicetree/bindings/riscv/microchip.yaml
@@ -21,6 +21,7 @@ properties:
       - enum:
           - microchip,mpfs-icicle-kit
           - microchip,mpfs-icicle-reference-rtlv2203
+          - microchip,mpfs-icicle-reference-rtlv2209
           - sundance,polarberry
       - const: microchip,mpfs
 
-- 
2.36.1

Re: [PATCH 5/6] dt-bindings: riscv: microchip: document icicle reference design

[Krzysztof Kozlowski]

On 19/08/2022 15:22, Conor Dooley wrote:
> Add a new compatible for the icicle kit reference design's 22.09
> release, which made some changes to the memory map - including adding
> the ability to read the CCCs via the system controller bus. Technically
> that was always possible, but the specific CC chosen could vary per
> run of the synthesis tool. Hopefully this is the last reference design
> version impacting the memory map.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

[PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Conor Dooley]

The "fabric clocks" in current PolarFire SoC device trees are not
really fixed clocks. Their frequency is set by the bitstream, so having
them located in -fabric.dtsi is not a problem - they're just as "fixed"
as the IP blocks etc used in the FPGA fabric.
However, their configuration can be read at runtime (and to an extent
they can be controlled, although the intended usage is static
configurations set by the bitstream) through the system controller bus.

In the v2209 reference design a single CCC (north-west corner) is
enabled, using a 50 MHz off-chip oscillator as its reference.

Updating to the v2209 reference design is required.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi | 27 +++++++++------
 .../boot/dts/microchip/mpfs-icicle-kit.dts    |  4 +++
 .../dts/microchip/mpfs-polarberry-fabric.dtsi |  5 +++
 arch/riscv/boot/dts/microchip/mpfs.dtsi       | 34 +++++++++++++++++--
 4 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
index 0d28858b83f2..f17cb00df467 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
@@ -2,14 +2,14 @@
 /* Copyright (c) 2020-2021 Microchip Technology Inc */
 
 / {
-	compatible = "microchip,mpfs-icicle-reference-rtlv2203", "microchip,mpfs";
+	compatible = "microchip,mpfs-icicle-reference-rtlv2209", "microchip,mpfs";
 
 	core_pwm0: pwm@41000000 {
 		compatible = "microchip,corepwm-rtl-v4";
 		reg = <0x0 0x41000000 0x0 0xF0>;
 		microchip,sync-update-mask = /bits/ 32 <0>;
 		#pwm-cells = <2>;
-		clocks = <&fabric_clk3>;
+		clocks = <&ccc_nw CLK_CCC_PLL0_OUT0>;
 		status = "disabled";
 	};
 
@@ -18,22 +18,29 @@ i2c2: i2c@44000000 {
 		reg = <0x0 0x44000000 0x0 0x1000>;
 		#address-cells = <1>;
 		#size-cells = <0>;
-		clocks = <&fabric_clk3>;
+		clocks = <&ccc_nw CLK_CCC_PLL0_OUT3>;
 		interrupt-parent = <&plic>;
 		interrupts = <122>;
 		clock-frequency = <100000>;
 		status = "disabled";
 	};
 
-	fabric_clk3: fabric-clk3 {
+	refclk_ccc: cccrefclk {
 		compatible = "fixed-clock";
 		#clock-cells = <0>;
-		clock-frequency = <62500000>;
 	};
+};
 
-	fabric_clk1: fabric-clk1 {
-		compatible = "fixed-clock";
-		#clock-cells = <0>;
-		clock-frequency = <125000000>;
-	};
+&ccc_nw {
+	clocks = <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>,
+		 <&refclk_ccc>, <&refclk_ccc>;
+	clock-names = "pll0_ref0", "pll0_ref1", "pll1_ref0", "pll1_ref1",
+		      "dll0_ref", "dll1_ref";
+	status = "okay";
+};
+
+&pcie {
+	clocks = <&ccc_nw CLK_CCC_PLL0_OUT0>, <&ccc_nw CLK_CCC_PLL0_OUT1>,
+		 <&ccc_nw CLK_CCC_PLL0_OUT3>;
+	clock-names = "fic0", "fic1", "fic3";
 };
diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
index 044982a11df5..d361d1e38b16 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
@@ -140,6 +140,10 @@ &refclk {
 	clock-frequency = <125000000>;
 };
 
+&refclk_ccc {
+	clock-frequency = <50000000>;
+};
+
 &rtc {
 	status = "okay";
 };
diff --git a/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi
index 49380c428ec9..3beb450b4259 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi
@@ -14,3 +14,8 @@ fabric_clk1: fabric-clk1 {
 		clock-frequency = <125000000>;
 	};
 };
+
+&pcie {
+	clocks = <&fabric_clk1>, <&fabric_clk1>, <&fabric_clk3>;
+	clock-names = "fic0", "fic1", "fic3";
+};
diff --git a/arch/riscv/boot/dts/microchip/mpfs.dtsi b/arch/riscv/boot/dts/microchip/mpfs.dtsi
index 499c2e63ad35..dd15b6d1a3c9 100644
--- a/arch/riscv/boot/dts/microchip/mpfs.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs.dtsi
@@ -236,6 +236,38 @@ clkcfg: clkcfg@20002000 {
 			#clock-cells = <1>;
 		};
 
+		ccc_se: cccseclk@38010000 {
+			compatible = "microchip,mpfs-ccc";
+			reg = <0x0 0x38010000 0x0 0x1000>, <0x0 0x38020000 0x0 0x1000>,
+			      <0x0 0x39010000 0x0 0x1000>, <0x0 0x39020000 0x0 0x1000>;
+			#clock-cells = <1>;
+			status = "disabled";
+		};
+
+		ccc_ne: cccneclk@38040000 {
+			compatible = "microchip,mpfs-ccc";
+			reg = <0x0 0x38040000 0x0 0x1000>, <0x0 0x38080000 0x0 0x1000>,
+			      <0x0 0x39040000 0x0 0x1000>, <0x0 0x39080000 0x0 0x1000>;
+			#clock-cells = <1>;
+			status = "disabled";
+		};
+
+		ccc_nw: cccnwclk@38100000 {
+			compatible = "microchip,mpfs-ccc";
+			reg = <0x0 0x38100000 0x0 0x1000>, <0x0 0x38200000 0x0 0x1000>,
+			      <0x0 0x39100000 0x0 0x1000>, <0x0 0x39200000 0x0 0x1000>;
+			#clock-cells = <1>;
+			status = "disabled";
+		};
+
+		ccc_sw: cccswclk@38400000 {
+			compatible = "microchip,mpfs-ccc";
+			reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>,
+			      <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>;
+			#clock-cells = <1>;
+			status = "disabled";
+		};
+
 		mmuart0: serial@20000000 {
 			compatible = "ns16550a";
 			reg = <0x0 0x20000000 0x0 0x400>;
@@ -480,8 +512,6 @@ pcie: pcie@2000000000 {
 					<0 0 0 3 &pcie_intc 2>,
 					<0 0 0 4 &pcie_intc 3>;
 			interrupt-map-mask = <0 0 0 7>;
-			clocks = <&fabric_clk1>, <&fabric_clk1>, <&fabric_clk3>;
-			clock-names = "fic0", "fic1", "fic3";
 			ranges = <0x3000000 0x0 0x8000000 0x20 0x8000000 0x0 0x80000000>;
 			msi-parent = <&pcie>;
 			msi-controller;
-- 
2.36.1

Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Krzysztof Kozlowski]

On 19/08/2022 15:23, Conor Dooley wrote:
> The "fabric clocks" in current PolarFire SoC device trees are not
> really fixed clocks. Their frequency is set by the bitstream, so having
> them located in -fabric.dtsi is not a problem - they're just as "fixed"
> as the IP blocks etc used in the FPGA fabric.
> However, their configuration can be read at runtime (and to an extent
> they can be controlled, although the intended usage is static
> configurations set by the bitstream) through the system controller bus.
> 

Thank you for your patch. There is something to discuss/improve.

> +&pcie {
> +	clocks = <&fabric_clk1>, <&fabric_clk1>, <&fabric_clk3>;
> +	clock-names = "fic0", "fic1", "fic3";
> +};
> diff --git a/arch/riscv/boot/dts/microchip/mpfs.dtsi b/arch/riscv/boot/dts/microchip/mpfs.dtsi
> index 499c2e63ad35..dd15b6d1a3c9 100644
> --- a/arch/riscv/boot/dts/microchip/mpfs.dtsi
> +++ b/arch/riscv/boot/dts/microchip/mpfs.dtsi
> @@ -236,6 +236,38 @@ clkcfg: clkcfg@20002000 {
>  			#clock-cells = <1>;
>  		};
>  
> +		ccc_se: cccseclk@38010000 {

Although you call it "Clock Conditioning Circuitry", but the role of
this device is a clock-controller, isn't it? If so, node names should be
generic, so "clock-controller".

https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


Best regards,
Krzysztof

Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Conor.Dooley]

On 19/08/2022 13:47, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 19/08/2022 15:23, Conor Dooley wrote:
>> The "fabric clocks" in current PolarFire SoC device trees are not
>> really fixed clocks. Their frequency is set by the bitstream, so having
>> them located in -fabric.dtsi is not a problem - they're just as "fixed"
>> as the IP blocks etc used in the FPGA fabric.
>> However, their configuration can be read at runtime (and to an extent
>> they can be controlled, although the intended usage is static
>> configurations set by the bitstream) through the system controller bus.
>>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +&pcie {
>> +     clocks = <&fabric_clk1>, <&fabric_clk1>, <&fabric_clk3>;
>> +     clock-names = "fic0", "fic1", "fic3";
>> +};
>> diff --git a/arch/riscv/boot/dts/microchip/mpfs.dtsi b/arch/riscv/boot/dts/microchip/mpfs.dtsi
>> index 499c2e63ad35..dd15b6d1a3c9 100644
>> --- a/arch/riscv/boot/dts/microchip/mpfs.dtsi
>> +++ b/arch/riscv/boot/dts/microchip/mpfs.dtsi
>> @@ -236,6 +236,38 @@ clkcfg: clkcfg@20002000 {
>>                        #clock-cells = <1>;
>>                };
>>
>> +             ccc_se: cccseclk@38010000 {
> 
> Although you call it "Clock Conditioning Circuitry", but the role of
> this device is a clock-controller, isn't it? If so, node names should be
> generic, so "clock-controller".

Thanks for the prompt reply Krzysztof!
I suspected that this is what I was going to hear back. The reason I
had used the non-generic node name is that I wanted to use it for the
"name" of the clocks in the clock framework. As you can see, there are
four instances of the same clock, and I am using the of_node's name to
generate the unique names the clock framework requires, like so:

# cat clk_summary
    clock
-------------------------
  cccrefclk
     cccnwclk_pll1
        cccnwclk_pll1_out3
        cccnwclk_pll1_out2
        cccnwclk_pll1_out1
        cccnwclk_pll1_out0
     cccnwclk_pll0
        cccnwclk_pll0_out3
        cccnwclk_pll0_out2
        cccnwclk_pll0_out1
        cccnwclk_pll0_out0
     cccswclk_pll1
        cccswclk_pll1_out3
        cccswclk_pll1_out2
        cccswclk_pll1_out1
        cccswclk_pll1_out0
     cccnsclk_pll0
        cccswclk_pll0_out3
        cccswclk_pll0_out2
        cccswclk_pll0_out1
        cccswclk_pll0_out0

Maybe that is me exploiting the "should", but I was not sure how to
include the location in the devicetree.

I had experimented with a "microchip,ordinal" or "microchip,location"
string property to do the same thing but I thought you/Rob might not
like that - is location/placement on the chip a relevant property of the
hardware? I'd argue that for an FPGA, where the user is the one deciding
what clocks what, it could be relevant to some degree.

Knowing if a CCC is the north-west one has some extra benefits as it
is co-located with the PLLs for the processor & has a reduced input
mux range.

Any suggestions would be appreciated, even if it is just a NAK to all of
the above!

Thanks,
Conor.

Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Krzysztof Kozlowski]

On 19/08/2022 16:15, Conor.Dooley@microchip.com wrote:
>>>
>>> +             ccc_se: cccseclk@38010000 {
>>
>> Although you call it "Clock Conditioning Circuitry", but the role of
>> this device is a clock-controller, isn't it? If so, node names should be
>> generic, so "clock-controller".
> 
> Thanks for the prompt reply Krzysztof!
> I suspected that this is what I was going to hear back. The reason I
> had used the non-generic node name is that I wanted to use it for the
> "name" of the clocks in the clock framework. As you can see, there are
> four instances of the same clock, and I am using the of_node's name to
> generate the unique names the clock framework requires, like so:
> 
> # cat clk_summary
>     clock
> -------------------------
>   cccrefclk
>      cccnwclk_pll1
>         cccnwclk_pll1_out3
>         cccnwclk_pll1_out2
>         cccnwclk_pll1_out1
>         cccnwclk_pll1_out0
>      cccnwclk_pll0
>         cccnwclk_pll0_out3
>         cccnwclk_pll0_out2
>         cccnwclk_pll0_out1
>         cccnwclk_pll0_out0
>      cccswclk_pll1
>         cccswclk_pll1_out3
>         cccswclk_pll1_out2
>         cccswclk_pll1_out1
>         cccswclk_pll1_out0
>      cccnsclk_pll0
>         cccswclk_pll0_out3
>         cccswclk_pll0_out2
>         cccswclk_pll0_out1
>         cccswclk_pll0_out0
> 
> Maybe that is me exploiting the "should", but I was not sure how to
> include the location in the devicetree.

Neither node names nor clock names are considered an ABI, but some
pieces like to rely on them. Now you created such dependency so imagine
someone prepares a DTSI/DTS with "clock-controller" names for all four
blocks. How you driver would behave?

The DTS would be perfectly valid but driver would not accept it
(conflicting names) or behave incorrect.

I think what you need is the clock-output-names property. The core
schema dtschema/schemas/clock/clock.yaml recommends unified
interpretation of it - list of names for all the clocks - but accepts
other uses, e.g. as a prefix.

> 
> I had experimented with a "microchip,ordinal" or "microchip,location"
> string property to do the same thing but I thought you/Rob might not
> like that - is location/placement on the chip a relevant property of the
> hardware? I'd argue that for an FPGA, where the user is the one deciding
> what clocks what, it could be relevant to some degree.
> 
> Knowing if a CCC is the north-west one has some extra benefits as it
> is co-located with the PLLs for the processor & has a reduced input
> mux range.
> 
> Any suggestions would be appreciated, even if it is just a NAK to all of
> the above!


Best regards,
Krzysztof

Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Conor.Dooley]

On 19/08/2022 14:28, Krzysztof Kozlowski wrote:
>> Maybe that is me exploiting the "should", but I was not sure how to
>> include the location in the devicetree.
> 
> Neither node names nor clock names are considered an ABI, but some
> pieces like to rely on them. Now you created such dependency so imagine
> someone prepares a DTSI/DTS with "clock-controller" names for all four
> blocks. How you driver would behave?

-EEXIST, registration fails in the core.

> The DTS would be perfectly valid but driver would not accept it
> (conflicting names) or behave incorrect.
> 
> I think what you need is the clock-output-names property. The core
> schema dtschema/schemas/clock/clock.yaml recommends unified
> interpretation of it - list of names for all the clocks - but accepts
> other uses, e.g. as a prefix.

So could I do `clock-output-names = "ccc_nw";`. That would work for me,
with one question:
How would I enforce the unique-ness of this property, since it would be
a per CCC/clock-controller property? Maybe I missed something, but I
gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check
did not complain. Up to me to explain the restriction in the dt-bindings
description?

FWIW I would then have:
ccc_sw: clock-controller@38400000 {
	compatible = "microchip,mpfs-ccc";
	reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>,
	      <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>;
	#clock-cells = <1>;
	clock-output-names = "ccc_sw";
	status = "disabled";
};

& in the binding:
   clock-output-names:
     pattern: ^ccc_[ns][ew]$

As always, thanks for your help. I did look at output names earlier in
the process, but didn't realise I could use it as a prefix.
Conor.

Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Krzysztof Kozlowski]

On 19/08/2022 16:48, Conor.Dooley@microchip.com wrote:
> On 19/08/2022 14:28, Krzysztof Kozlowski wrote:
>>> Maybe that is me exploiting the "should", but I was not sure how to
>>> include the location in the devicetree.
>>
>> Neither node names nor clock names are considered an ABI, but some
>> pieces like to rely on them. Now you created such dependency so imagine
>> someone prepares a DTSI/DTS with "clock-controller" names for all four
>> blocks. How you driver would behave?
> 
> -EEXIST, registration fails in the core.
> 
>> The DTS would be perfectly valid but driver would not accept it
>> (conflicting names) or behave incorrect.
>>
>> I think what you need is the clock-output-names property. The core
>> schema dtschema/schemas/clock/clock.yaml recommends unified
>> interpretation of it - list of names for all the clocks - but accepts
>> other uses, e.g. as a prefix.
> 
> So could I do `clock-output-names = "ccc_nw";`. That would work for me,
> with one question:
> How would I enforce the unique-ness of this property, since it would be
> a per CCC/clock-controller property? Maybe I missed something, but I
> gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check
> did not complain. Up to me to explain the restriction in the dt-bindings
> description?

Uniqueness among entire DTS? I don't think you can, except of course
mentioning it in description. Your driver should handle such DTS -
minimally by gracefully failing but better behaving in some default way.

> 
> FWIW I would then have:
> ccc_sw: clock-controller@38400000 {
> 	compatible = "microchip,mpfs-ccc";
> 	reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>,
> 	      <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>;
> 	#clock-cells = <1>;
> 	clock-output-names = "ccc_sw";
> 	status = "disabled";
> };
> 
> & in the binding:
>    clock-output-names:
>      pattern: ^ccc_[ns][ew]$

Yes, although this won't enforce uniqueness.

Best regards,
Krzysztof

Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Conor.Dooley]

On 19/08/2022 15:06, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 19/08/2022 16:48, Conor.Dooley@microchip.com wrote:
>> On 19/08/2022 14:28, Krzysztof Kozlowski wrote:
>>>> Maybe that is me exploiting the "should", but I was not sure how to
>>>> include the location in the devicetree.
>>>
>>> Neither node names nor clock names are considered an ABI, but some
>>> pieces like to rely on them. Now you created such dependency so imagine
>>> someone prepares a DTSI/DTS with "clock-controller" names for all four
>>> blocks. How you driver would behave?
>>
>> -EEXIST, registration fails in the core.
>>
>>> The DTS would be perfectly valid but driver would not accept it
>>> (conflicting names) or behave incorrect.
>>>
>>> I think what you need is the clock-output-names property. The core
>>> schema dtschema/schemas/clock/clock.yaml recommends unified
>>> interpretation of it - list of names for all the clocks - but accepts
>>> other uses, e.g. as a prefix.
>>
>> So could I do `clock-output-names = "ccc_nw";`. That would work for me,
>> with one question:
>> How would I enforce the unique-ness of this property, since it would be
>> a per CCC/clock-controller property? Maybe I missed something, but I
>> gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check
>> did not complain. Up to me to explain the restriction in the dt-bindings
>> description?
> 
> Uniqueness among entire DTS? I don't think you can, except of course
> mentioning it in description. Your driver should handle such DTS -
> minimally by gracefully failing but better behaving in some default way.

It fails not-too-gracefully at the moment, but that could easily be
changed. Truncated base address I suppose would be a meaningful thing
to fall back to afterwards.

> 
>>
>> FWIW I would then have:
>> ccc_sw: clock-controller@38400000 {
>>        compatible = "microchip,mpfs-ccc";
>>        reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>,
>>              <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>;
>>        #clock-cells = <1>;
>>        clock-output-names = "ccc_sw";
>>        status = "disabled";
>> };
>>
>> & in the binding:
>>     clock-output-names:
>>       pattern: ^ccc_[ns][ew]$
> 
> Yes, although this won't enforce uniqueness.

I know :( I'll respin next week I guess, thanks again.
Conor.

Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Krzysztof Kozlowski]

On 19/08/2022 17:14, Conor.Dooley@microchip.com wrote:
> On 19/08/2022 15:06, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 19/08/2022 16:48, Conor.Dooley@microchip.com wrote:
>>> On 19/08/2022 14:28, Krzysztof Kozlowski wrote:
>>>>> Maybe that is me exploiting the "should", but I was not sure how to
>>>>> include the location in the devicetree.
>>>>
>>>> Neither node names nor clock names are considered an ABI, but some
>>>> pieces like to rely on them. Now you created such dependency so imagine
>>>> someone prepares a DTSI/DTS with "clock-controller" names for all four
>>>> blocks. How you driver would behave?
>>>
>>> -EEXIST, registration fails in the core.
>>>
>>>> The DTS would be perfectly valid but driver would not accept it
>>>> (conflicting names) or behave incorrect.
>>>>
>>>> I think what you need is the clock-output-names property. The core
>>>> schema dtschema/schemas/clock/clock.yaml recommends unified
>>>> interpretation of it - list of names for all the clocks - but accepts
>>>> other uses, e.g. as a prefix.
>>>
>>> So could I do `clock-output-names = "ccc_nw";`. That would work for me,
>>> with one question:
>>> How would I enforce the unique-ness of this property, since it would be
>>> a per CCC/clock-controller property? Maybe I missed something, but I
>>> gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check
>>> did not complain. Up to me to explain the restriction in the dt-bindings
>>> description?
>>
>> Uniqueness among entire DTS? I don't think you can, except of course
>> mentioning it in description. Your driver should handle such DTS -
>> minimally by gracefully failing but better behaving in some default way.
> 
> It fails not-too-gracefully at the moment, but that could easily be
> changed. Truncated base address I suppose would be a meaningful thing
> to fall back to afterwards.
> 
>>
>>>
>>> FWIW I would then have:
>>> ccc_sw: clock-controller@38400000 {
>>>        compatible = "microchip,mpfs-ccc";
>>>        reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>,
>>>              <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>;
>>>        #clock-cells = <1>;
>>>        clock-output-names = "ccc_sw";
>>>        status = "disabled";
>>> };
>>>
>>> & in the binding:
>>>     clock-output-names:
>>>       pattern: ^ccc_[ns][ew]$
>>
>> Yes, although this won't enforce uniqueness.
> 
> I know :( I'll respin next week I guess, thanks again.


The issue with this is that we are getting close to tying bindings with
your Linux implementation. What if other implementation does not use
these names as prefix and instead creates some other clock names (as
clock names are not considered ABI)? Your binding would still enforce
such property with a specific pattern.

The clock names should not really matter, so if you have conflict of
names among multiple controllers, I think driver should embed unit
address in the name (as fallback of clock-output-name) and the binding
should not enforce specific pattern.

I can easily imagine a real hardware board design with
"sexy_duck_ccc_pll1_out3" clock names. :)

Best regards,
Krzysztof

Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Conor.Dooley]

On 19/08/2022 15:22, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 19/08/2022 17:14, Conor.Dooley@microchip.com wrote:
>> On 19/08/2022 15:06, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 19/08/2022 16:48, Conor.Dooley@microchip.com wrote:
>>>> On 19/08/2022 14:28, Krzysztof Kozlowski wrote:
>>>>>> Maybe that is me exploiting the "should", but I was not sure how to
>>>>>> include the location in the devicetree.
>>>>>
>>>>> Neither node names nor clock names are considered an ABI, but some
>>>>> pieces like to rely on them. Now you created such dependency so imagine
>>>>> someone prepares a DTSI/DTS with "clock-controller" names for all four
>>>>> blocks. How you driver would behave?
>>>>
>>>> -EEXIST, registration fails in the core.
>>>>
>>>>> The DTS would be perfectly valid but driver would not accept it
>>>>> (conflicting names) or behave incorrect.
>>>>>
>>>>> I think what you need is the clock-output-names property. The core
>>>>> schema dtschema/schemas/clock/clock.yaml recommends unified
>>>>> interpretation of it - list of names for all the clocks - but accepts
>>>>> other uses, e.g. as a prefix.
>>>>
>>>> So could I do `clock-output-names = "ccc_nw";`. That would work for me,
>>>> with one question:
>>>> How would I enforce the unique-ness of this property, since it would be
>>>> a per CCC/clock-controller property? Maybe I missed something, but I
>>>> gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check
>>>> did not complain. Up to me to explain the restriction in the dt-bindings
>>>> description?
>>>
>>> Uniqueness among entire DTS? I don't think you can, except of course
>>> mentioning it in description. Your driver should handle such DTS -
>>> minimally by gracefully failing but better behaving in some default way.
>>
>> It fails not-too-gracefully at the moment, but that could easily be
>> changed. Truncated base address I suppose would be a meaningful thing
>> to fall back to afterwards.
>>
>>>
>>>>
>>>> FWIW I would then have:
>>>> ccc_sw: clock-controller@38400000 {
>>>>         compatible = "microchip,mpfs-ccc";
>>>>         reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>,
>>>>               <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>;
>>>>         #clock-cells = <1>;
>>>>         clock-output-names = "ccc_sw";
>>>>         status = "disabled";
>>>> };
>>>>
>>>> & in the binding:
>>>>      clock-output-names:
>>>>        pattern: ^ccc_[ns][ew]$
>>>
>>> Yes, although this won't enforce uniqueness.
>>
>> I know :( I'll respin next week I guess, thanks again.
> 
> 
> The issue with this is that we are getting close to tying bindings with
> your Linux implementation. What if other implementation does not use
> these names as prefix and instead creates some other clock names (as
> clock names are not considered ABI)? Your binding would still enforce
> such property with a specific pattern.
> 
> The clock names should not really matter, so if you have conflict of
> names among multiple controllers, I think driver should embed unit
> address in the name (as fallback of clock-output-name) and the binding
> should not enforce specific pattern.

Not sure if you just passed over it, but I agree:
>> Truncated base address I suppose would be a meaningful thing
>> to fall back to afterwards.

But if the names aren't an ABI, then either there's not much point in
having the regex at all for clock-output-names or failing the check for
it does not matter. I'll have a think over the weekend about what
exactly to do, but I think the driver side of this is clear to me now &
what not to do in the binding is too.

> I can easily imagine a real hardware board design with
> "sexy_duck_ccc_pll1_out3" clock names. :)

If Alestorm made a board with our FPGA, I could see that..
I'd buy the t-shirt too!

Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

[Krzysztof Kozlowski]

On 19/08/2022 17:32, Conor.Dooley@microchip.com wrote:
>> The clock names should not really matter, so if you have conflict of
>> names among multiple controllers, I think driver should embed unit
>> address in the name (as fallback of clock-output-name) and the binding
>> should not enforce specific pattern.
> 
> Not sure if you just passed over it, but I agree:
>>> Truncated base address I suppose would be a meaningful thing
>>> to fall back to afterwards.

Yeah, indeed, you mentioned it.

> 
> But if the names aren't an ABI, then either there's not much point in
> having the regex at all for clock-output-names or failing the check for
> it does not matter. I'll have a think over the weekend about what
> exactly to do, but I think the driver side of this is clear to me now &
> what not to do in the binding is too.

Yes.

> 
>> I can easily imagine a real hardware board design with
>> "sexy_duck_ccc_pll1_out3" clock names. :)
> 
> If Alestorm made a board with our FPGA, I could see that..
> I'd buy the t-shirt too!
> 


Best regards,
Krzysztof