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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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 <[email protected]>");
+MODULE_LICENSE("GPL");
--
2.36.1
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 <[email protected]>
---
.../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
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 <[email protected]>
---
.../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 <[email protected]>
+
+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
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
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 <[email protected]>
> ---
> .../dt-bindings/clock/microchip,mpfs-clock.h | 23 +++++++++++++++++++
This could be also a separate header file for CCC.
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
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 <[email protected]>
> ---
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
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 <[email protected]>
Best regards,
Krzysztof
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
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.
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.
On 19/08/2022 16:15, [email protected] 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
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.
On 19/08/2022 17:14, [email protected] 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, [email protected] 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
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, [email protected] 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.
On 19/08/2022 16:48, [email protected] 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
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, [email protected] 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, [email protected] 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!
On 19/08/2022 17:32, [email protected] 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