[PATCH 0/2] iio: frequency: add iio support for Amlogic clock measure

[PATCH 0/2] iio: frequency: add iio support for Amlogic clock measure

[Jerome Brunet]

Add support for the HW found in most Amlogic SoC dedicated to measure
system clocks.

This drivers aims to replace the one found in
drivers/soc/amlogic/meson-clk-measure.c with following improvements:

* Access to the measurements through the IIO API:
  Easier re-use of the results in userspace and other drivers
* Controllable scale with raw measurements
* Higher precision with processed measurements

Jerome Brunet (2):
  dt-bindings: iio: frequency: add clock measure support
  iio: frequency: add amlogic clock measure support

 .../iio/frequency/amlogic,clk-msr-io.yaml     |  50 ++
 drivers/iio/frequency/Kconfig                 |  15 +
 drivers/iio/frequency/Makefile                |   1 +
 drivers/iio/frequency/amlogic-clk-msr-io.c    | 802 ++++++++++++++++++
 4 files changed, 868 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
 create mode 100644 drivers/iio/frequency/amlogic-clk-msr-io.c

-- 
2.43.0

[PATCH 1/2] dt-bindings: iio: frequency: add clock measure support

[Jerome Brunet]

Introduce new bindings for the clock measure IP found Amlogic SoCs.
These new bindings help bring IIO support to AML clock measure.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---

 NOTE:
  #1: Splitting the register space looks odd. This is done to help support
      future SoCs. From meson8 to sm1, duty register comes first, then the
      reg space. On s4 and c3, duty comes after the reg space.
  #2: 'reg' may look like a poor choice of name. This comes from the
       documentation.
       - MSR_CLK_REGx for the 'reg' space (x being a number)
       - MSR_CLK_DUTY for the 'duty' space

 .../iio/frequency/amlogic,clk-msr-io.yaml     | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml

diff --git a/Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml b/Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
new file mode 100644
index 000000000000..eeb268b4a607
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/frequency/amlogic,clk-msr-io.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic System Clock Measurer
+
+description:
+  Internal clock rate sensor within Amlogic SoCs
+
+maintainers:
+  - Neil Armstrong <neil.armstrong@linaro.org>
+
+properties:
+  compatible:
+    enum:
+      - amlogic,meson8-clk-msr-io
+      - amlogic,gx-clk-msr-io
+      - amlogic,axg-clk-msr-io
+      - amlogic,g12a-clk-msr-io
+      - amlogic,sm1-clk-msr-io
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: reg
+      - const: duty
+
+  "#io-channel-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    clk_msr: rate-sensor@18000 {
+      compatible = "amlogic,axg-clk-msr-io";
+      reg = <0x18004 0xc>,
+            <0x18000 0x4>;
+      reg-names = "reg", "duty";
+      #io-channel-cells = <1>;
+    };
-- 
2.43.0

Re: [PATCH 1/2] dt-bindings: iio: frequency: add clock measure support

[Krzysztof Kozlowski]

On 24/06/2024 19:31, Jerome Brunet wrote:
> Introduce new bindings for the clock measure IP found Amlogic SoCs.
> These new bindings help bring IIO support to AML clock measure.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline), work on fork of kernel
(don't, instead use mainline) or you ignore some maintainers (really
don't). Just use b4 and everything should be fine, although remember
about `b4 prep --auto-to-cc` if you added new patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling.

Please kindly resend and include all necessary To/Cc entries.
</form letter>



Best regards,
Krzysztof

[PATCH 2/2] iio: frequency: add amlogic clock measure support

[Jerome Brunet]

Add support for the HW found in most Amlogic SoC dedicated to measure
system clocks.

This drivers aims to replace the one found in
drivers/soc/amlogic/meson-clk-measure.c with following improvements:

* Access to the measurements through the IIO API:
  Easier re-use of the results in userspace and other drivers
* Controllable scale with raw measurements
* Higher precision with processed measurements

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/iio/frequency/Kconfig              |  15 +
 drivers/iio/frequency/Makefile             |   1 +
 drivers/iio/frequency/amlogic-clk-msr-io.c | 802 +++++++++++++++++++++
 3 files changed, 818 insertions(+)
 create mode 100644 drivers/iio/frequency/amlogic-clk-msr-io.c

diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
index c455be7d4a1c..1682cbec0edf 100644
--- a/drivers/iio/frequency/Kconfig
+++ b/drivers/iio/frequency/Kconfig
@@ -7,6 +7,21 @@
 #
 # When adding new entries keep the list in alphabetical order
 
+menu "Frequency Measurements"
+
+config AMLOGIC_CLK_MSR_IO
+	tristate "Amlogic IO Clock Measure"
+	default ARCH_MESON
+	depends on REGMAP_MMIO && COMMON_CLK
+	help
+	  Say yes here to build support for Amlogic Clock Measure
+	  HW found in Amlogic SoC, with IIO support
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called amlogic-clk-msr-io.
+
+endmenu
+
 menu "Frequency Synthesizers DDS/PLL"
 
 menu "Clock Generator/Distribution"
diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
index 70d0e0b70e80..3939970cccf3 100644
--- a/drivers/iio/frequency/Makefile
+++ b/drivers/iio/frequency/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_ADMV1013) += admv1013.o
 obj-$(CONFIG_ADMV1014) += admv1014.o
 obj-$(CONFIG_ADMV4420) += admv4420.o
 obj-$(CONFIG_ADRF6780) += adrf6780.o
+obj-$(CONFIG_AMLOGIC_CLK_MSR_IO) += amlogic-clk-msr-io.o
diff --git a/drivers/iio/frequency/amlogic-clk-msr-io.c b/drivers/iio/frequency/amlogic-clk-msr-io.c
new file mode 100644
index 000000000000..16a15dd346d8
--- /dev/null
+++ b/drivers/iio/frequency/amlogic-clk-msr-io.c
@@ -0,0 +1,802 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2024 BayLibre, SAS.
+// Author: Jerome Brunet <jbrunet@baylibre.com>
+
+#include <linux/bitfield.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define MSR_CLK_REG0	0x0
+#define  MSR_BUSY	BIT(31)
+#define  MSR_CLK_SRC	GENMASK(26, 20)
+#define  MSR_CLK_EN	BIT(19)
+#define  MSR_CONT	BIT(17)
+#define  MSR_ENABLE	BIT(16)
+#define  MSR_TIME	GENMASK(15, 0)
+#define   MSR_TIME_MAX	(MSR_TIME + 1)
+#define MSR_CLK_REG1	0x4
+#define MSR_CLK_REG2	0x8
+#define  MSR_MEASURED	GENMASK(19, 0)
+
+#define CLK_MIN_TIME 64 /* This allows to measure up to 8GHz */
+#define CLK_MSR_MAX 128
+
+/*
+ * Note this driver aims to replace drivers/soc/amlogic/meson-clk-measure.c
+ * It provides the same functionality and adds support for the IIO API.
+ * The only thing missing is the clock summary which is very handy for
+ * debugging purpose. It is easy to reproduce the summary in userspace,
+ * from the iio device sysfs directory:
+ *
+ * for i in $(seq 0 127); do
+ *   printf "%20s: %11dHz +/- %.0fHz\n" \
+ *           $(cat in_altvoltage${i}_label) \
+ *           $(cat in_altvoltage${i}_input) \
+ *           $(cat in_altvoltage_scale);
+ * done
+ */
+
+struct amlogic_cmsr {
+	struct regmap *reg;
+	struct regmap *duty;
+	struct mutex lock;
+};
+
+static const char *cmsr_m8[CLK_MSR_MAX] = {
+	[0]  = "ring_osc_out_ee0",
+	[1]  = "ring_osc_out_ee1",
+	[2]  = "ring_osc_out_ee2",
+	[3]  = "a9_ring_osck",
+	[6]  = "vid_pll",
+	[7]  = "clk81",
+	[8]  = "encp",
+	[9]  = "encl",
+	[11] = "eth_rmii",
+	[13] = "amclk",
+	[14] = "fec_clk_0",
+	[15] = "fec_clk_1",
+	[16] = "fec_clk_2",
+	[18] = "a9_clk_div16",
+	[19] = "hdmi_sys",
+	[20] = "rtc_osc_clk_out",
+	[21] = "i2s_clk_in_src0",
+	[22] = "clk_rmii_from_pad",
+	[23] = "hdmi_ch0_tmds",
+	[24] = "lvds_fifo",
+	[26] = "sc_clk_int",
+	[28] = "sar_adc",
+	[30] = "mpll_clk_test_out",
+	[31] = "audac_clkpi",
+	[32] = "vdac",
+	[33] = "sdhc_rx",
+	[34] = "sdhc_sd",
+	[35] = "mali",
+	[36] = "hdmi_tx_pixel",
+	[38] = "vdin_meas",
+	[39] = "pcm_sclk",
+	[40] = "pcm_mclk",
+	[41] = "eth_rx_tx",
+	[42] = "pwm_d",
+	[43] = "pwm_c",
+	[44] = "pwm_b",
+	[45] = "pwm_a",
+	[46] = "pcm2_sclk",
+	[47] = "ddr_dpll_pt",
+	[48] = "pwm_f",
+	[49] = "pwm_e",
+	[59] = "hcodec",
+	[60] = "usb_32k_alt",
+	[61] = "gpio",
+	[62] = "vid2_pll",
+	[63] = "mipi_csi_cfg",
+};
+
+static const char *cmsr_gx[CLK_MSR_MAX] = {
+	[0]  = "ring_osc_out_ee_0",
+	[1]  = "ring_osc_out_ee_1",
+	[2]  = "ring_osc_out_ee_2",
+	[3]  = "a53_ring_osc",
+	[4]  = "gp0_pll",
+	[6]  = "enci",
+	[7]  = "clk81",
+	[8]  = "encp",
+	[9]  = "encl",
+	[10] = "vdac",
+	[11] = "rgmii_tx",
+	[12] = "pdm",
+	[13] = "amclk",
+	[14] = "fec_0",
+	[15] = "fec_1",
+	[16] = "fec_2",
+	[17] = "sys_pll_div16",
+	[18] = "sys_cpu_div16",
+	[19] = "hdmitx_sys",
+	[20] = "rtc_osc_out",
+	[21] = "i2s_in_src0",
+	[22] = "eth_phy_ref",
+	[23] = "hdmi_todig",
+	[26] = "sc_int",
+	[28] = "sar_adc",
+	[31] = "mpll_test_out",
+	[32] = "vdec",
+	[35] = "mali",
+	[36] = "hdmi_tx_pixel",
+	[37] = "i958",
+	[38] = "vdin_meas",
+	[39] = "pcm_sclk",
+	[40] = "pcm_mclk",
+	[41] = "eth_rx_or_rmii",
+	[42] = "mp0_out",
+	[43] = "fclk_div5",
+	[44] = "pwm_b",
+	[45] = "pwm_a",
+	[46] = "vpu",
+	[47] = "ddr_dpll_pt",
+	[48] = "mp1_out",
+	[49] = "mp2_out",
+	[50] = "mp3_out",
+	[51] = "nand_core",
+	[52] = "sd_emmc_b",
+	[53] = "sd_emmc_a",
+	[55] = "vid_pll_div_out",
+	[56] = "cci",
+	[57] = "wave420l_c",
+	[58] = "wave420l_b",
+	[59] = "hcodec",
+	[60] = "alt_32k",
+	[61] = "gpio_msr",
+	[62] = "hevc",
+	[66] = "vid_lock",
+	[70] = "pwm_f",
+	[71] = "pwm_e",
+	[72] = "pwm_d",
+	[73] = "pwm_c",
+	[75] = "aoclkx2_int",
+	[76] = "aoclk_int",
+	[77] = "rng_ring_osc_0",
+	[78] = "rng_ring_osc_1",
+	[79] = "rng_ring_osc_2",
+	[80] = "rng_ring_osc_3",
+	[81] = "vapb",
+	[82] = "ge2d",
+};
+
+static const char *cmsr_axg[CLK_MSR_MAX] = {
+	[0]   = "ring_osc_out_ee_0",
+	[1]   = "ring_osc_out_ee_1",
+	[2]   = "ring_osc_out_ee_2",
+	[3]   = "a53_ring_osc",
+	[4]   = "gp0_pll",
+	[5]   = "gp1_pll",
+	[7]   = "clk81",
+	[9]   = "encl",
+	[17]  = "sys_pll_div16",
+	[18]  = "sys_cpu_div16",
+	[20]  = "rtc_osc_out",
+	[23]  = "mmc_clk",
+	[28]  = "sar_adc",
+	[31]  = "mpll_test_out",
+	[40]  = "mod_eth_tx_clk",
+	[41]  = "mod_eth_rx_clk_rmii",
+	[42]  = "mp0_out",
+	[43]  = "fclk_div5",
+	[44]  = "pwm_b",
+	[45]  = "pwm_a",
+	[46]  = "vpu",
+	[47]  = "ddr_dpll_pt",
+	[48]  = "mp1_out",
+	[49]  = "mp2_out",
+	[50]  = "mp3_out",
+	[51]  = "sd_emmm_c",
+	[52]  = "sd_emmc_b",
+	[61]  = "gpio_msr",
+	[66]  = "audio_slv_lrclk_c",
+	[67]  = "audio_slv_lrclk_b",
+	[68]  = "audio_slv_lrclk_a",
+	[69]  = "audio_slv_sclk_c",
+	[70]  = "audio_slv_sclk_b",
+	[71]  = "audio_slv_sclk_a",
+	[72]  = "pwm_d",
+	[73]  = "pwm_c",
+	[74]  = "wifi_beacon",
+	[75]  = "tdmin_lb_lrcl",
+	[76]  = "tdmin_lb_sclk",
+	[77]  = "rng_ring_osc_0",
+	[78]  = "rng_ring_osc_1",
+	[79]  = "rng_ring_osc_2",
+	[80]  = "rng_ring_osc_3",
+	[81]  = "vapb",
+	[82]  = "ge2d",
+	[84]  = "audio_resample",
+	[85]  = "audio_pdm_sys",
+	[86]  = "audio_spdifout",
+	[87]  = "audio_spdifin",
+	[88]  = "audio_lrclk_f",
+	[89]  = "audio_lrclk_e",
+	[90]  = "audio_lrclk_d",
+	[91]  = "audio_lrclk_c",
+	[92]  = "audio_lrclk_b",
+	[93]  = "audio_lrclk_a",
+	[94]  = "audio_sclk_f",
+	[95]  = "audio_sclk_e",
+	[96]  = "audio_sclk_d",
+	[97]  = "audio_sclk_c",
+	[98]  = "audio_sclk_b",
+	[99]  = "audio_sclk_a",
+	[100] = "audio_mclk_f",
+	[101] = "audio_mclk_e",
+	[102] = "audio_mclk_d",
+	[103] = "audio_mclk_c",
+	[104] = "audio_mclk_b",
+	[105] = "audio_mclk_a",
+	[106] = "pcie_refclk_n",
+	[107] = "pcie_refclk_p",
+	[108] = "audio_locker_out",
+	[109] = "audio_locker_in",
+};
+
+static const char *cmsr_g12a[CLK_MSR_MAX] = {
+	[0]   = "ring_osc_out_ee_0",
+	[1]   = "ring_osc_out_ee_1",
+	[2]   = "ring_osc_out_ee_2",
+	[3]   = "sys_cpu_ring_osc",
+	[4]   = "gp0_pll",
+	[6]   = "enci",
+	[7]   = "clk81",
+	[8]   = "encp",
+	[9]   = "encl",
+	[10]  = "vdac",
+	[11]  = "eth_tx",
+	[12]  = "hifi_pll",
+	[13]  = "mod_tcon",
+	[14]  = "fec_0",
+	[15]  = "fec_1",
+	[16]  = "fec_2",
+	[17]  = "sys_pll_div16",
+	[18]  = "sys_cpu_div16",
+	[19]  = "lcd_an_ph2",
+	[20]  = "rtc_osc_out",
+	[21]  = "lcd_an_ph3",
+	[22]  = "eth_phy_ref",
+	[23]  = "mpll_50m",
+	[24]  = "eth_125m",
+	[25]  = "eth_rmii",
+	[26]  = "sc_int",
+	[27]  = "in_mac",
+	[28]  = "sar_adc",
+	[29]  = "pcie_inp",
+	[30]  = "pcie_inn",
+	[31]  = "mpll_test_out",
+	[32]  = "vdec",
+	[33]  = "sys_cpu_ring_osc_1",
+	[34]  = "eth_mpll_50m",
+	[35]  = "mali",
+	[36]  = "hdmi_tx_pixel",
+	[37]  = "cdac",
+	[38]  = "vdin_meas",
+	[39]  = "bt656",
+	[41]  = "eth_rx_or_rmii",
+	[42]  = "mp0_out",
+	[43]  = "fclk_div5",
+	[44]  = "pwm_b",
+	[45]  = "pwm_a",
+	[46]  = "vpu",
+	[47]  = "ddr_dpll_pt",
+	[48]  = "mp1_out",
+	[49]  = "mp2_out",
+	[50]  = "mp3_out",
+	[51]  = "sd_emmc_c",
+	[52]  = "sd_emmc_b",
+	[53]  = "sd_emmc_a",
+	[54]  = "vpu_clkc",
+	[55]  = "vid_pll_div_out",
+	[56]  = "wave420l_a",
+	[57]  = "wave420l_c",
+	[58]  = "wave420l_b",
+	[59]  = "hcodec",
+	[61]  = "gpio_msr",
+	[62]  = "hevcb",
+	[63]  = "dsi_meas",
+	[64]  = "spicc_1",
+	[65]  = "spicc_0",
+	[66]  = "vid_lock",
+	[67]  = "dsi_phy",
+	[68]  = "hdcp22_esm",
+	[69]  = "hdcp22_skp",
+	[70]  = "pwm_f",
+	[71]  = "pwm_e",
+	[72]  = "pwm_d",
+	[73]  = "pwm_c",
+	[75]  = "hevcf",
+	[77]  = "rng_ring_osc_0",
+	[78]  = "rng_ring_osc_1",
+	[79]  = "rng_ring_osc_2",
+	[80]  = "rng_ring_osc_3",
+	[81]  = "vapb",
+	[82]  = "ge2d",
+	[83]  = "co_rx",
+	[84]  = "co_tx",
+	[89]  = "hdmi_todig",
+	[90]  = "hdmitx_sys",
+	[91]  = "sys_cpub_div16",
+	[92]  = "sys_pll_cpub_div16",
+	[94]  = "eth_phy_rx",
+	[95]  = "eth_phy_pll",
+	[96]  = "vpu_b",
+	[97]  = "cpu_b_tmp",
+	[98]  = "ts",
+	[99]  = "ring_osc_out_ee_3",
+	[100] = "ring_osc_out_ee_4",
+	[101] = "ring_osc_out_ee_5",
+	[102] = "ring_osc_out_ee_6",
+	[103] = "ring_osc_out_ee_7",
+	[104] = "ring_osc_out_ee_8",
+	[105] = "ring_osc_out_ee_9",
+	[106] = "ephy_test",
+	[107] = "au_dac_g128x",
+	[108] = "audio_locker_out",
+	[109] = "audio_locker_in",
+	[110] = "audio_tdmout_c_sclk",
+	[111] = "audio_tdmout_b_sclk",
+	[112] = "audio_tdmout_a_sclk",
+	[113] = "audio_tdmin_lb_sclk",
+	[114] = "audio_tdmin_c_sclk",
+	[115] = "audio_tdmin_b_sclk",
+	[116] = "audio_tdmin_a_sclk",
+	[117] = "audio_resample",
+	[118] = "audio_pdm_sys",
+	[119] = "audio_spdifout_b",
+	[120] = "audio_spdifout",
+	[121] = "audio_spdifin",
+	[122] = "audio_pdm_dclk",
+};
+
+static const char *cmsr_sm1[CLK_MSR_MAX] = {
+	[0]   = "ring_osc_out_ee_0",
+	[1]   = "ring_osc_out_ee_1",
+	[2]   = "ring_osc_out_ee_2",
+	[3]   = "ring_osc_out_ee_3",
+	[4]   = "gp0_pll",
+	[5]   = "gp1_pll",
+	[6]   = "enci",
+	[7]   = "clk81",
+	[8]   = "encp",
+	[9]   = "encl",
+	[10]  = "vdac",
+	[11]  = "eth_tx",
+	[12]  = "hifi_pll",
+	[13]  = "mod_tcon",
+	[14]  = "fec_0",
+	[15]  = "fec_1",
+	[16]  = "fec_2",
+	[17]  = "sys_pll_div16",
+	[18]  = "sys_cpu_div16",
+	[19]  = "lcd_an_ph2",
+	[20]  = "rtc_osc_out",
+	[21]  = "lcd_an_ph3",
+	[22]  = "eth_phy_ref",
+	[23]  = "mpll_50m",
+	[24]  = "eth_125m",
+	[25]  = "eth_rmii",
+	[26]  = "sc_int",
+	[27]  = "in_mac",
+	[28]  = "sar_adc",
+	[29]  = "pcie_inp",
+	[30]  = "pcie_inn",
+	[31]  = "mpll_test_out",
+	[32]  = "vdec",
+	[34]  = "eth_mpll_50m",
+	[35]  = "mali",
+	[36]  = "hdmi_tx_pixel",
+	[37]  = "cdac",
+	[38]  = "vdin_meas",
+	[39]  = "bt656",
+	[40]  = "arm_ring_osc_out_4",
+	[41]  = "eth_rx_or_rmii",
+	[42]  = "mp0_out",
+	[43]  = "fclk_div5",
+	[44]  = "pwm_b",
+	[45]  = "pwm_a",
+	[46]  = "vpu",
+	[47]  = "ddr_dpll_pt",
+	[48]  = "mp1_out",
+	[49]  = "mp2_out",
+	[50]  = "mp3_out",
+	[51]  = "sd_emmc_c",
+	[52]  = "sd_emmc_b",
+	[53]  = "sd_emmc_a",
+	[54]  = "vpu_clkc",
+	[55]  = "vid_pll_div_out",
+	[56]  = "wave420l_a",
+	[57]  = "wave420l_c",
+	[58]  = "wave420l_b",
+	[59]  = "hcodec",
+	[60]  = "arm_ring_osc_out_5",
+	[61]  = "gpio_msr",
+	[62]  = "hevcb",
+	[63]  = "dsi_meas",
+	[64]  = "spicc_1",
+	[65]  = "spicc_0",
+	[66]  = "vid_lock",
+	[67]  = "dsi_phy",
+	[68]  = "hdcp22_esm",
+	[69]  = "hdcp22_skp",
+	[70]  = "pwm_f",
+	[71]  = "pwm_e",
+	[72]  = "pwm_d",
+	[73]  = "pwm_c",
+	[74]  = "arm_ring_osc_out_6",
+	[75]  = "hevcf",
+	[76]  = "arm_ring_osc_out_7",
+	[77]  = "rng_ring_osc_0",
+	[78]  = "rng_ring_osc_1",
+	[79]  = "rng_ring_osc_2",
+	[80]  = "rng_ring_osc_3",
+	[81]  = "vapb",
+	[82]  = "ge2d",
+	[83]  = "co_rx",
+	[84]  = "co_tx",
+	[85]  = "arm_ring_osc_out_8",
+	[86]  = "arm_ring_osc_out_9",
+	[87]  = "mipi_dsi_phy",
+	[88]  = "cis2_adapt",
+	[89]  = "hdmi_todig",
+	[90]  = "hdmitx_sys",
+	[91]  = "nna_core",
+	[92]  = "nna_axi",
+	[93]  = "vad",
+	[94]  = "eth_phy_rx",
+	[95]  = "eth_phy_pll",
+	[96]  = "vpu_b",
+	[97]  = "cpu_b_tmp",
+	[98]  = "ts",
+	[99]  = "arm_ring_osc_out_10",
+	[100] = "arm_ring_osc_out_11",
+	[101] = "arm_ring_osc_out_12",
+	[102] = "arm_ring_osc_out_13",
+	[103] = "arm_ring_osc_out_14",
+	[104] = "arm_ring_osc_out_15",
+	[105] = "arm_ring_osc_out_16",
+	[106] = "ephy_test",
+	[107] = "au_dac_g128x",
+	[108] = "audio_locker_out",
+	[109] = "audio_locker_in",
+	[110] = "audio_tdmout_c_sclk",
+	[111] = "audio_tdmout_b_sclk",
+	[112] = "audio_tdmout_a_sclk",
+	[113] = "audio_tdmin_lb_sclk",
+	[114] = "audio_tdmin_c_sclk",
+	[115] = "audio_tdmin_b_sclk",
+	[116] = "audio_tdmin_a_sclk",
+	[117] = "audio_resample",
+	[118] = "audio_pdm_sys",
+	[119] = "audio_spdifout_b",
+	[120] = "audio_spdifout",
+	[121] = "audio_spdifin",
+	[122] = "audio_pdm_dclk",
+	[123] = "audio_resampled",
+	[124] = "earcrx_pll",
+	[125] = "earcrx_pll_test",
+	[126] = "csi_phy0",
+	[127] = "csi2_data",
+};
+
+static struct iio_chan_spec *cmsr_populate_channels(struct device *dev,
+						    const char * const *conf)
+{
+	struct iio_chan_spec *chan;
+	int i;
+
+	chan = devm_kzalloc(dev, sizeof(*chan) * CLK_MSR_MAX, GFP_KERNEL);
+	if (!chan)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < CLK_MSR_MAX; i++) {
+		chan[i].type = IIO_ALTVOLTAGE;
+		chan[i].indexed = 1;
+		chan[i].channel = i;
+		chan[i].info_mask_separate = (BIT(IIO_CHAN_INFO_RAW) |
+					    BIT(IIO_CHAN_INFO_PROCESSED));
+		chan[i].info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
+		chan[i].datasheet_name = conf[i];
+	}
+
+	return chan;
+}
+
+static int cmsr_get_time_unlocked(struct amlogic_cmsr *cm)
+{
+	unsigned int raw;
+
+	regmap_read(cm->reg, MSR_CLK_REG0, &raw);
+
+	/* Field register value is time = val + 1 */
+	return FIELD_GET(MSR_TIME, raw) + 1;
+}
+
+static int cmsr_set_time_unlocked(struct amlogic_cmsr *cm,
+				  unsigned int time)
+{
+	time -= 1;
+
+	if (time < 0 || time > MSR_TIME)
+		return -EINVAL;
+
+	regmap_update_bits(cm->reg, MSR_CLK_REG0, MSR_TIME,
+			   FIELD_PREP(MSR_TIME, time));
+
+	return 0;
+}
+
+static int cmsr_measure_unlocked(struct amlogic_cmsr *cm,
+				 unsigned int idx)
+{
+	unsigned int raw;
+	int ret;
+
+	regmap_update_bits(cm->reg, MSR_CLK_REG0,
+			   MSR_ENABLE | MSR_CONT | MSR_CLK_EN | MSR_CLK_SRC,
+			   MSR_CLK_EN | FIELD_PREP(MSR_CLK_SRC, idx));
+
+	regmap_set_bits(cm->reg, MSR_CLK_REG0, MSR_ENABLE);
+	ret = regmap_read_poll_timeout(cm->reg, MSR_CLK_REG0, raw,
+				       !(raw & MSR_BUSY), 10, 70000);
+	regmap_clear_bits(cm->reg, MSR_CLK_REG0, MSR_ENABLE | MSR_CLK_EN);
+
+	if (ret)
+		return ret;
+
+	regmap_read(cm->reg, MSR_CLK_REG2, &raw);
+	ret = FIELD_GET(MSR_MEASURED, raw);
+
+	/* Check for overflow */
+	if (ret == MSR_MEASURED)
+		return -EINVAL;
+
+	return ret;
+}
+
+static int cmsr_measure_processed_unlocked(struct amlogic_cmsr *cm,
+					   unsigned int idx,
+					   int *val2)
+{
+	unsigned int time = CLK_MIN_TIME;
+	u64 rate;
+	int ret;
+
+	/*
+	 * The challenge here is to provide the best accuracy
+	 * while not spending to much time doing it.
+	 * - Starting with a short duration risk not detecting
+	 *   slow clocks, but it is fast. All 128 can be done in ~8ms
+	 * - Starting with a long duration risk overflowing the
+	 *   measurement counter and would be way to long, especially
+	 *   considering the number of disabled clocks. ~4s for all
+	 *   128 worst case.
+	 *
+	 * This IP measures system clocks so all clock are expected
+	 * to be 1kHz < f < 8GHz. We can compromise based on this,
+	 * doing it in 3 pass:
+	 * #1 Starting if 64us window: detects 30kHz < f < 8GHz
+	 *    - Go to #2 if no detection, Go to #3 otherwise
+	 * #2 Extend duration to 1024us (f > 1kHz)
+	      - Assume f = 0Hz if no detection, Go to #3 otherwise
+	 * #3 Clock has been detected, adjust window for best accuracy
+	 *
+	 * Doing the range detection takes ~1ms per clock, including disabled
+	   clocks.
+	 * Actual measurement takes at most ~65ms in step #3 for slow clocks,
+	 * when the full range the HW is used.
+	 */
+
+	/* Step #1 - quick measurement */
+	cmsr_set_time_unlocked(cm, time);
+	ret = cmsr_measure_unlocked(cm, idx);
+	if (ret < 0)
+		return ret;
+
+	else if (ret == 0) {
+		/* Step #2 - extend the window if necessary */
+		time *= 16;
+		cmsr_set_time_unlocked(cm, time);
+		ret = cmsr_measure_unlocked(cm, idx);
+		if (ret < 0)
+			return ret;
+
+		else if (ret == 0) {
+			/* Still nothing - assume no clock */
+			*val2 = 0;
+			return 0;
+		}
+	}
+
+	/* Step #3: Adapt scale for better precision */
+	time = time * MSR_MEASURED * 3 / (ret * 4); /* 25% margin */
+	time = min_t(unsigned int, MSR_TIME_MAX, time);
+
+	/* Actually measure rate with an optimized scale */
+	cmsr_set_time_unlocked(cm, time);
+	ret = cmsr_measure_unlocked(cm, idx);
+	if (ret < 0)
+		return ret;
+
+	rate = DIV_ROUND_CLOSEST_ULL(ret * 1000000ULL, time);
+	*val2 = rate >> 32ULL;
+	return (int)rate;
+}
+
+static int cmsr_read_raw(struct iio_dev *indio_dev,
+			 struct iio_chan_spec const *chan,
+			 int *val, int *val2, long mask)
+{
+	struct amlogic_cmsr *cm = iio_priv(indio_dev);
+
+	guard(mutex)(&cm->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*val = cmsr_measure_unlocked(cm, chan->channel);
+		if (*val < 0)
+			return *val;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_PROCESSED: /* Result in Hz */
+		*val = cmsr_measure_processed_unlocked(cm, chan->channel, val2);
+		if (*val < 0)
+			return *val;
+		return IIO_VAL_INT_64;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val2 = cmsr_get_time_unlocked(cm);
+		*val = 1000000;
+		return IIO_VAL_FRACTIONAL;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int cmsr_write_raw(struct iio_dev *indio_dev,
+			  struct iio_chan_spec const *chan,
+			  int val, int val2, long info)
+{
+	struct amlogic_cmsr *cm = iio_priv(indio_dev);
+	unsigned int time;
+
+	guard(mutex)(&cm->lock);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		time = DIV_ROUND_CLOSEST(1000000U, val);
+		return cmsr_set_time_unlocked(cm, time);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int cmsr_write_raw_get_fmt(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan,
+				  long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT;
+	default:
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+}
+
+static int cmsr_read_label(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   char *label)
+{
+	return sprintf(label, "%s\n", chan->datasheet_name);
+}
+
+static const struct of_device_id cmsr_of_match[] = {
+	{
+		.compatible = "amlogic,gx-clk-msr-io",
+		.data = cmsr_gx,
+	}, {
+		.compatible = "amlogic,meson8-clk-msr-io",
+		.data = cmsr_m8,
+	}, {
+		.compatible = "amlogic,axg-clk-msr-io",
+		.data = cmsr_axg,
+	}, {
+		.compatible = "amlogic,g12a-clk-msr-io",
+		.data = cmsr_g12a,
+	}, {
+		.compatible = "amlogic,sm1-clk-msr-io",
+		.data = cmsr_sm1,
+	}, {}
+};
+MODULE_DEVICE_TABLE(of, cmsr_of_match);
+
+static const struct iio_info cmsr_info = {
+	.read_raw = cmsr_read_raw,
+	.read_label = cmsr_read_label,
+	.write_raw = cmsr_write_raw,
+	.write_raw_get_fmt = cmsr_write_raw_get_fmt,
+};
+
+static const struct regmap_config cmsr_regmap_cfg = {
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+};
+
+static int cmsr_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct iio_dev *indio_dev;
+	struct amlogic_cmsr *cm;
+	const char * const *conf;
+	void __iomem *regs;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*cm));
+	if (!indio_dev)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, indio_dev);
+	cm = iio_priv(indio_dev);
+
+	conf = of_device_get_match_data(dev);
+	if (!conf) {
+		dev_err(dev, "failed to match device\n");
+		return -ENODEV;
+	}
+
+	regs = devm_platform_ioremap_resource_byname(pdev, "reg");
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	cm->reg = devm_regmap_init_mmio(dev, regs, &cmsr_regmap_cfg);
+	if (IS_ERR(cm->reg)) {
+		dev_err(dev, "failed to init main regmap: %ld\n",
+			PTR_ERR(cm->reg));
+		return PTR_ERR(cm->reg);
+	}
+
+	regs = devm_platform_ioremap_resource_byname(pdev, "duty");
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	cm->duty = devm_regmap_init_mmio(dev, regs, &cmsr_regmap_cfg);
+	if (IS_ERR(cm->duty)) {
+		dev_err(dev, "failed to init duty regmap: %ld\n",
+			PTR_ERR(cm->duty));
+		return PTR_ERR(cm->duty);
+	}
+
+	mutex_init(&cm->lock);
+
+	/* Init scale with a sane default */
+	cmsr_set_time_unlocked(cm, CLK_MIN_TIME);
+
+	indio_dev->name = "amlogic-clk-msr";
+	indio_dev->info = &cmsr_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = CLK_MSR_MAX;
+	indio_dev->channels = cmsr_populate_channels(dev, conf);
+	if (IS_ERR(indio_dev->channels))
+		return PTR_ERR(indio_dev->channels);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static struct platform_driver amlogic_cmsr_driver = {
+	.probe		= cmsr_probe,
+	.driver		= {
+		.name	= "amlogic-clk-msr-io",
+		.of_match_table = cmsr_of_match,
+	},
+};
+module_platform_driver(amlogic_cmsr_driver);
+
+MODULE_DESCRIPTION("Amlogic Clock Measure IO driver");
+MODULE_AUTHOR("Jerome Brunet <jbrunet@baylibre.com>");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

[David Lechner]

On 6/24/24 12:31 PM, Jerome Brunet wrote:
> Add support for the HW found in most Amlogic SoC dedicated to measure
> system clocks.
> 



> +static int cmsr_read_raw(struct iio_dev *indio_dev,
> +			 struct iio_chan_spec const *chan,
> +			 int *val, int *val2, long mask)
> +{
> +	struct amlogic_cmsr *cm = iio_priv(indio_dev);
> +
> +	guard(mutex)(&cm->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = cmsr_measure_unlocked(cm, chan->channel);

Is this actually returning an alternating voltage magnitutde?
Most frequency drivers don't have a raw value, only frequency.

> +		if (*val < 0)
> +			return *val;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_PROCESSED: /* Result in Hz */

Shouldn't this be IIO_CHAN_INFO_FREQUENCY?

Processed is just (raw + offset) * scale which would be a voltage
in this case since the channel type is IIO_ALTVOLTAGE.

> +		*val = cmsr_measure_processed_unlocked(cm, chan->channel, val2);
> +		if (*val < 0)
> +			return *val;
> +		return IIO_VAL_INT_64;
> +
> +	case IIO_CHAN_INFO_SCALE:

What is this attribute being used for?

(clearly not used to convert the raw value to millivolts :-) )

Maybe IIO_CHAN_INFO_INT_TIME is the right one for this? Although
so far, that has only been used with light sensors.

> +		*val2 = cmsr_get_time_unlocked(cm);
> +		*val = 1000000;
> +		return IIO_VAL_FRACTIONAL;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

[David Lechner]

On 6/24/24 5:51 PM, David Lechner wrote:
> On 6/24/24 12:31 PM, Jerome Brunet wrote:
>> Add support for the HW found in most Amlogic SoC dedicated to measure
>> system clocks.
>>
> 
> 
> 
>> +static int cmsr_read_raw(struct iio_dev *indio_dev,
>> +			 struct iio_chan_spec const *chan,
>> +			 int *val, int *val2, long mask)
>> +{
>> +	struct amlogic_cmsr *cm = iio_priv(indio_dev);
>> +
>> +	guard(mutex)(&cm->lock);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		*val = cmsr_measure_unlocked(cm, chan->channel);
> 
> Is this actually returning an alternating voltage magnitutde?
> Most frequency drivers don't have a raw value, only frequency.
> 
>> +		if (*val < 0)
>> +			return *val;
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_PROCESSED: /* Result in Hz */
> 
> Shouldn't this be IIO_CHAN_INFO_FREQUENCY?
> 
> Processed is just (raw + offset) * scale which would be a voltage
> in this case since the channel type is IIO_ALTVOLTAGE.
> 
>> +		*val = cmsr_measure_processed_unlocked(cm, chan->channel, val2);
>> +		if (*val < 0)
>> +			return *val;
>> +		return IIO_VAL_INT_64;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
> 
> What is this attribute being used for?
> 
> (clearly not used to convert the raw value to millivolts :-) )
> 
> Maybe IIO_CHAN_INFO_INT_TIME is the right one for this? Although
> so far, that has only been used with light sensors.

Probably not the right idea though since it applies to the frequency
measurement, not the voltage measurement.

> 
>> +		*val2 = cmsr_get_time_unlocked(cm);
>> +		*val = 1000000;
>> +		return IIO_VAL_FRACTIONAL;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +

Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

[Jerome Brunet]

On Mon 24 Jun 2024 at 17:51, David Lechner <dlechner@baylibre.com> wrote:

> On 6/24/24 12:31 PM, Jerome Brunet wrote:
>> Add support for the HW found in most Amlogic SoC dedicated to measure
>> system clocks.
>> 
>
>
>
>> +static int cmsr_read_raw(struct iio_dev *indio_dev,
>> +			 struct iio_chan_spec const *chan,
>> +			 int *val, int *val2, long mask)
>> +{
>> +	struct amlogic_cmsr *cm = iio_priv(indio_dev);
>> +
>> +	guard(mutex)(&cm->lock);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		*val = cmsr_measure_unlocked(cm, chan->channel);
>
> Is this actually returning an alternating voltage magnitutde?
> Most frequency drivers don't have a raw value, only frequency.

No it is not the magnitude, it is the clock rate (frequency) indeed.
Maybe altvoltage was not the right pick for that but nothing obvious
stands out for Hz measurements

>
>> +		if (*val < 0)
>> +			return *val;
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_PROCESSED: /* Result in Hz */
>
> Shouldn't this be IIO_CHAN_INFO_FREQUENCY?

How would I get raw / processed / scale with IIO_CHAN_INFO_FREQUENCY ?

>
> Processed is just (raw + offset) * scale which would be a voltage
> in this case since the channel type is IIO_ALTVOLTAGE.

This is was Processed does here, along with selecting the most
appropriate scale to perform the measurement.

>
>> +		*val = cmsr_measure_processed_unlocked(cm, chan->channel, val2);
>> +		if (*val < 0)
>> +			return *val;
>> +		return IIO_VAL_INT_64;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>
> What is this attribute being used for?

Hz

>
> (clearly not used to convert the raw value to millivolts :-) )
>
> Maybe IIO_CHAN_INFO_INT_TIME is the right one for this? Although
> so far, that has only been used with light sensors.

I think you are mixing up channel info and type here.
I do want the info
 * IIO_CHAN_INFO_RAW
 * IIO_CHAN_INFO_PROCESSED
 * IIO_CHAN_INFO_SCALE

I want those info to represent an alternate voltage frequency in Hz.
I thought type 'IIO_ALTVOLTAGE' was the right pick for that. Apparently
it is not. What is the appropriate type then ? Should I add a new one ?

>
>> +		*val2 = cmsr_get_time_unlocked(cm);
>> +		*val = 1000000;
>> +		return IIO_VAL_FRACTIONAL;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +

-- 
Jerome

Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

[David Lechner]

On 6/25/24 3:31 AM, Jerome Brunet wrote:
> On Mon 24 Jun 2024 at 17:51, David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 6/24/24 12:31 PM, Jerome Brunet wrote:
>>> Add support for the HW found in most Amlogic SoC dedicated to measure
>>> system clocks.
>>>
>>
>>
>>
>>> +static int cmsr_read_raw(struct iio_dev *indio_dev,
>>> +			 struct iio_chan_spec const *chan,
>>> +			 int *val, int *val2, long mask)
>>> +{
>>> +	struct amlogic_cmsr *cm = iio_priv(indio_dev);
>>> +
>>> +	guard(mutex)(&cm->lock);
>>> +
>>> +	switch (mask) {
>>> +	case IIO_CHAN_INFO_RAW:
>>> +		*val = cmsr_measure_unlocked(cm, chan->channel);
>>
>> Is this actually returning an alternating voltage magnitutde?
>> Most frequency drivers don't have a raw value, only frequency.
> 
> No it is not the magnitude, it is the clock rate (frequency) indeed.
> Maybe altvoltage was not the right pick for that but nothing obvious
> stands out for Hz measurements

I'm certainly not an expert on the subject, but looking at the other
frequency drivers, using altvoltage looks correct.

But, we in those drivers, nearly all only have a "frequency" attribute
but don't have a "raw" attribute. The ones that do have a "raw" attribute
are frequency generators that use the raw attribute determine the output
voltage.

> 
>>
>>> +		if (*val < 0)
>>> +			return *val;
>>> +		return IIO_VAL_INT;
>>> +
>>> +	case IIO_CHAN_INFO_PROCESSED: /* Result in Hz */
>>
>> Shouldn't this be IIO_CHAN_INFO_FREQUENCY?
> 
> How would I get raw / processed / scale with IIO_CHAN_INFO_FREQUENCY ?
> 
>>
>> Processed is just (raw + offset) * scale which would be a voltage
>> in this case since the channel type is IIO_ALTVOLTAGE.
> 
> This is was Processed does here, along with selecting the most
> appropriate scale to perform the measurement.
> 
>>
>>> +		*val = cmsr_measure_processed_unlocked(cm, chan->channel, val2);
>>> +		if (*val < 0)
>>> +			return *val;
>>> +		return IIO_VAL_INT_64;
>>> +
>>> +	case IIO_CHAN_INFO_SCALE:
>>
>> What is this attribute being used for?
> 
> Hz
> 
>>
>> (clearly not used to convert the raw value to millivolts :-) )
>>
>> Maybe IIO_CHAN_INFO_INT_TIME is the right one for this? Although
>> so far, that has only been used with light sensors.
> 
> I think you are mixing up channel info and type here.
> I do want the info
>  * IIO_CHAN_INFO_RAW
>  * IIO_CHAN_INFO_PROCESSED
>  * IIO_CHAN_INFO_SCALE
> 
> I want those info to represent an alternate voltage frequency in Hz.
> I thought type 'IIO_ALTVOLTAGE' was the right pick for that. Apparently
> it is not. What is the appropriate type then ? Should I add a new one ?


The documentation at Documentation/ABI/testing/sysfs-bus-iio explains
what the combination of a channel type and info means.

For example, out_altvoltageY_raw is defined as it used for the frequency
generator case that I mentioned above. in_altvoltageY_raw is not defined
which means probably no one has needed it yet. But it would still be the
voltage value, not the frequency.

There also isn't a in_altvoltageY_input defined ("_input" is the sysfs
name for IIO_CHAN_INFO_PROCESSED). But in general, "input" (processed) in
IIO just means the value in standard units instead of a raw value. It
doesn't mean that any extra signal processing was done. And "scale" is
just a way to convert "raw" to "input" (processed). So channels will either
have "raw" and "scale" or only have "input" (processed), but not both.

Could you describe at a higher level what the use case for the various
values being exposed here are? I think that would be helpful in figuring
out where they actually fit in to the standard IIO attributes.

> 
>>
>>> +		*val2 = cmsr_get_time_unlocked(cm);
>>> +		*val = 1000000;
>>> +		return IIO_VAL_FRACTIONAL;
>>> +
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +}
>>> +
> 

Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

[Jerome Brunet]

On Tue 25 Jun 2024 at 09:52, David Lechner <dlechner@baylibre.com> wrote:

> On 6/25/24 3:31 AM, Jerome Brunet wrote:
>> On Mon 24 Jun 2024 at 17:51, David Lechner <dlechner@baylibre.com> wrote:
>> 
>>> On 6/24/24 12:31 PM, Jerome Brunet wrote:
>>>> Add support for the HW found in most Amlogic SoC dedicated to measure
>>>> system clocks.
>>>>
>>>
>>>
>>>
>>>> +static int cmsr_read_raw(struct iio_dev *indio_dev,
>>>> +			 struct iio_chan_spec const *chan,
>>>> +			 int *val, int *val2, long mask)
>>>> +{
>>>> +	struct amlogic_cmsr *cm = iio_priv(indio_dev);
>>>> +
>>>> +	guard(mutex)(&cm->lock);
>>>> +
>>>> +	switch (mask) {
>>>> +	case IIO_CHAN_INFO_RAW:
>>>> +		*val = cmsr_measure_unlocked(cm, chan->channel);
>>>
>>> Is this actually returning an alternating voltage magnitutde?
>>> Most frequency drivers don't have a raw value, only frequency.
>> 
>> No it is not the magnitude, it is the clock rate (frequency) indeed.
>> Maybe altvoltage was not the right pick for that but nothing obvious
>> stands out for Hz measurements
>
> I'm certainly not an expert on the subject, but looking at the other
> frequency drivers, using altvoltage looks correct.
>
> But, we in those drivers, nearly all only have a "frequency" attribute
> but don't have a "raw" attribute. The ones that do have a "raw" attribute
> are frequency generators that use the raw attribute determine the output
> voltage.
>
>> 
>>>
>>>> +		if (*val < 0)
>>>> +			return *val;
>>>> +		return IIO_VAL_INT;
>>>> +
>>>> +	case IIO_CHAN_INFO_PROCESSED: /* Result in Hz */
>>>
>>> Shouldn't this be IIO_CHAN_INFO_FREQUENCY?
>> 
>> How would I get raw / processed / scale with IIO_CHAN_INFO_FREQUENCY ?
>> 
>>>
>>> Processed is just (raw + offset) * scale which would be a voltage
>>> in this case since the channel type is IIO_ALTVOLTAGE.
>> 
>> This is was Processed does here, along with selecting the most
>> appropriate scale to perform the measurement.
>> 
>>>
>>>> +		*val = cmsr_measure_processed_unlocked(cm, chan->channel, val2);
>>>> +		if (*val < 0)
>>>> +			return *val;
>>>> +		return IIO_VAL_INT_64;
>>>> +
>>>> +	case IIO_CHAN_INFO_SCALE:
>>>
>>> What is this attribute being used for?
>> 
>> Hz
>> 
>>>
>>> (clearly not used to convert the raw value to millivolts :-) )
>>>
>>> Maybe IIO_CHAN_INFO_INT_TIME is the right one for this? Although
>>> so far, that has only been used with light sensors.
>> 
>> I think you are mixing up channel info and type here.
>> I do want the info
>>  * IIO_CHAN_INFO_RAW
>>  * IIO_CHAN_INFO_PROCESSED
>>  * IIO_CHAN_INFO_SCALE
>> 
>> I want those info to represent an alternate voltage frequency in Hz.
>> I thought type 'IIO_ALTVOLTAGE' was the right pick for that. Apparently
>> it is not. What is the appropriate type then ? Should I add a new one ?
>
>
> The documentation at Documentation/ABI/testing/sysfs-bus-iio explains
> what the combination of a channel type and info means.

Oh missed that, Thx

>
> For example, out_altvoltageY_raw is defined as it used for the frequency
> generator case that I mentioned above. in_altvoltageY_raw is not defined
> which means probably no one has needed it yet. But it would still be the
> voltage value, not the frequency.

Got it. So the type I picked is wrong for sure.
So, maybe I need something new to measure a frequency ?

>
> There also isn't a in_altvoltageY_input defined ("_input" is the sysfs
> name for IIO_CHAN_INFO_PROCESSED). But in general, "input" (processed) in
> IIO just means the value in standard units instead of a raw value. It
> doesn't mean that any extra signal processing was done. And "scale" is
> just a way to convert "raw" to "input" (processed). So channels will either
> have "raw" and "scale" or only have "input" (processed), but not both.
>
> Could you describe at a higher level what the use case for the various
> values being exposed here are? I think that would be helpful in figuring
> out where they actually fit in to the standard IIO attributes.

So as discussed in the cover letter, This HW used to be driven by a driver
living in drivers/soc/amlogic/meson-clk-measure.c .
It provides debugfs entries to observe the rate of 128 system clocks.
It was fine until now and mostly used for debug in userspace.

To solve a problem with implementing eARC support in ASoC, I need
another driver to access the measurements, as simply as possible.
IIO provides an interface like this and the HW actually does
measurements so I seems like a good fit. I've got it to work
and happy with the result, both on the IIO and ASoC side.

What is really important to me is the information provided with
IIO_CHAN_INFO_PROCESSED, because the algorithm will adapt the scale (the
duration the clock is observed for) to not overflow the output and get the
best precision achievable. It is a compromise really. Consumer do not
have to worry about it, they'll get Hz.

I'd prefer to avoid having that algorithm (duplicated) in the consumers,
especially if it is a driver.

I've provided RAW and SCALE because it is cheap and easy to do TBH.
I can live without it. It is meant for consumers (if any?) that would
know better, like want to prioritize speed over precision.

Eventually that driver could evolve and also provide duty cycle
measurements, which is another reason why I think IIO is a good fit for
it.

>
>> 
>>>
>>>> +		*val2 = cmsr_get_time_unlocked(cm);
>>>> +		*val = 1000000;
>>>> +		return IIO_VAL_FRACTIONAL;
>>>> +
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>> +}
>>>> +
>> 

-- 
Jerome

Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

[Jonathan Cameron]

On Tue, 25 Jun 2024 18:59:58 +0200
Jerome Brunet <jbrunet@baylibre.com> wrote:

> On Tue 25 Jun 2024 at 09:52, David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 6/25/24 3:31 AM, Jerome Brunet wrote:  
> >> On Mon 24 Jun 2024 at 17:51, David Lechner <dlechner@baylibre.com> wrote:
> >>   
> >>> On 6/24/24 12:31 PM, Jerome Brunet wrote:  
> >>>> Add support for the HW found in most Amlogic SoC dedicated to measure
> >>>> system clocks.
> >>>>  
> >>>
> >>>
> >>>  
> >>>> +static int cmsr_read_raw(struct iio_dev *indio_dev,
> >>>> +			 struct iio_chan_spec const *chan,
> >>>> +			 int *val, int *val2, long mask)
> >>>> +{
> >>>> +	struct amlogic_cmsr *cm = iio_priv(indio_dev);
> >>>> +
> >>>> +	guard(mutex)(&cm->lock);
> >>>> +
> >>>> +	switch (mask) {
> >>>> +	case IIO_CHAN_INFO_RAW:
> >>>> +		*val = cmsr_measure_unlocked(cm, chan->channel);  
> >>>
> >>> Is this actually returning an alternating voltage magnitutde?
> >>> Most frequency drivers don't have a raw value, only frequency.  
> >> 
> >> No it is not the magnitude, it is the clock rate (frequency) indeed.
> >> Maybe altvoltage was not the right pick for that but nothing obvious
> >> stands out for Hz measurements  
> >
> > I'm certainly not an expert on the subject, but looking at the other
> > frequency drivers, using altvoltage looks correct.
> >
> > But, we in those drivers, nearly all only have a "frequency" attribute
> > but don't have a "raw" attribute. The ones that do have a "raw" attribute
> > are frequency generators that use the raw attribute determine the output
> > voltage.
> >  
> >>   
> >>>  
> >>>> +		if (*val < 0)
> >>>> +			return *val;
> >>>> +		return IIO_VAL_INT;
> >>>> +
> >>>> +	case IIO_CHAN_INFO_PROCESSED: /* Result in Hz */  
> >>>
> >>> Shouldn't this be IIO_CHAN_INFO_FREQUENCY?  
> >> 
> >> How would I get raw / processed / scale with IIO_CHAN_INFO_FREQUENCY ?
> >>   
> >>>
> >>> Processed is just (raw + offset) * scale which would be a voltage
> >>> in this case since the channel type is IIO_ALTVOLTAGE.  
> >> 
> >> This is was Processed does here, along with selecting the most
> >> appropriate scale to perform the measurement.
> >>   
> >>>  
> >>>> +		*val = cmsr_measure_processed_unlocked(cm, chan->channel, val2);
> >>>> +		if (*val < 0)
> >>>> +			return *val;
> >>>> +		return IIO_VAL_INT_64;
> >>>> +
> >>>> +	case IIO_CHAN_INFO_SCALE:  
> >>>
> >>> What is this attribute being used for?  
> >> 
> >> Hz
> >>   
> >>>
> >>> (clearly not used to convert the raw value to millivolts :-) )
> >>>
> >>> Maybe IIO_CHAN_INFO_INT_TIME is the right one for this? Although
> >>> so far, that has only been used with light sensors.  
> >> 
> >> I think you are mixing up channel info and type here.
> >> I do want the info
> >>  * IIO_CHAN_INFO_RAW
> >>  * IIO_CHAN_INFO_PROCESSED
> >>  * IIO_CHAN_INFO_SCALE
> >> 
> >> I want those info to represent an alternate voltage frequency in Hz.
> >> I thought type 'IIO_ALTVOLTAGE' was the right pick for that. Apparently
> >> it is not. What is the appropriate type then ? Should I add a new one ?  
> >
> >
> > The documentation at Documentation/ABI/testing/sysfs-bus-iio explains
> > what the combination of a channel type and info means.  
> 
> Oh missed that, Thx
> 
> >
> > For example, out_altvoltageY_raw is defined as it used for the frequency
> > generator case that I mentioned above. in_altvoltageY_raw is not defined
> > which means probably no one has needed it yet. But it would still be the
> > voltage value, not the frequency.  
> 
> Got it. So the type I picked is wrong for sure.
> So, maybe I need something new to measure a frequency ?

Yes. Seems likely we need a new channel type to me.
In theory we could abuse an angular rate channel but that's nasty so
lets not do that. :)

Jonathan

Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

[Jonathan Cameron]

On Mon, 24 Jun 2024 19:31:03 +0200
Jerome Brunet <jbrunet@baylibre.com> wrote:

> Add support for the HW found in most Amlogic SoC dedicated to measure
> system clocks.
> 
> This drivers aims to replace the one found in
> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
> 
> * Access to the measurements through the IIO API:
>   Easier re-use of the results in userspace and other drivers
> * Controllable scale with raw measurements
> * Higher precision with processed measurements
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
Interesting device and the driver is pretty clean.

Needs a new channel type though as altvoltage is in volts not hz.

Various minor comments inline.

Thanks,

Jonathan

> +
> +struct amlogic_cmsr {
> +	struct regmap *reg;
> +	struct regmap *duty;
> +	struct mutex lock;
Add a comment on the data protected by this lock.  Scope isn't always obvious.
> +};

> +static struct iio_chan_spec *cmsr_populate_channels(struct device *dev,
> +						    const char * const *conf)
> +{
> +	struct iio_chan_spec *chan;
> +	int i;
> +
> +	chan = devm_kzalloc(dev, sizeof(*chan) * CLK_MSR_MAX, GFP_KERNEL);
> +	if (!chan)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < CLK_MSR_MAX; i++) {
> +		chan[i].type = IIO_ALTVOLTAGE;

As per other branch of the thread.  I think this needs to be a new
IIO_FREQUENCY channel as the input we care about here is simply frequency,
not the voltage.

> +		chan[i].indexed = 1;
> +		chan[i].channel = i;
> +		chan[i].info_mask_separate = (BIT(IIO_CHAN_INFO_RAW) |
> +					    BIT(IIO_CHAN_INFO_PROCESSED));

Drop outer ()

> +		chan[i].info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> +		chan[i].datasheet_name = conf[i];
> +	}
> +
> +	return chan;
> +}


> +static int cmsr_measure_processed_unlocked(struct amlogic_cmsr *cm,
> +					   unsigned int idx,
> +					   int *val2)
> +{
> +	unsigned int time = CLK_MIN_TIME;
> +	u64 rate;
> +	int ret;
> +
> +	/*
> +	 * The challenge here is to provide the best accuracy
> +	 * while not spending to much time doing it.
> +	 * - Starting with a short duration risk not detecting
> +	 *   slow clocks, but it is fast. All 128 can be done in ~8ms
> +	 * - Starting with a long duration risk overflowing the
> +	 *   measurement counter and would be way to long, especially
> +	 *   considering the number of disabled clocks. ~4s for all
> +	 *   128 worst case.
> +	 *
> +	 * This IP measures system clocks so all clock are expected
> +	 * to be 1kHz < f < 8GHz. We can compromise based on this,
> +	 * doing it in 3 pass:
> +	 * #1 Starting if 64us window: detects 30kHz < f < 8GHz
> +	 *    - Go to #2 if no detection, Go to #3 otherwise
> +	 * #2 Extend duration to 1024us (f > 1kHz)
> +	      - Assume f = 0Hz if no detection, Go to #3 otherwise
> +	 * #3 Clock has been detected, adjust window for best accuracy
> +	 *
> +	 * Doing the range detection takes ~1ms per clock, including disabled
> +	   clocks.
> +	 * Actual measurement takes at most ~65ms in step #3 for slow clocks,
> +	 * when the full range the HW is used.
> +	 */
> +
> +	/* Step #1 - quick measurement */
> +	cmsr_set_time_unlocked(cm, time);
> +	ret = cmsr_measure_unlocked(cm, idx);
> +	if (ret < 0)
> +		return ret;
> +

if (ret == 0)

> +	else if (ret == 0) {
> +		/* Step #2 - extend the window if necessary */
> +		time *= 16;
> +		cmsr_set_time_unlocked(cm, time);
> +		ret = cmsr_measure_unlocked(cm, idx);
> +		if (ret < 0)
> +			return ret;
> +
		if (ret == 0)

> +		else if (ret == 0) {
> +			/* Still nothing - assume no clock */
> +			*val2 = 0;
> +			return 0;
> +		}
> +	}
> +
> +	/* Step #3: Adapt scale for better precision */
> +	time = time * MSR_MEASURED * 3 / (ret * 4); /* 25% margin */
> +	time = min_t(unsigned int, MSR_TIME_MAX, time);
> +
> +	/* Actually measure rate with an optimized scale */
> +	cmsr_set_time_unlocked(cm, time);
> +	ret = cmsr_measure_unlocked(cm, idx);
> +	if (ret < 0)
> +		return ret;
> +
> +	rate = DIV_ROUND_CLOSEST_ULL(ret * 1000000ULL, time);
> +	*val2 = rate >> 32ULL;
> +	return (int)rate;
> +}



> +static int cmsr_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct amlogic_cmsr *cm;
> +	const char * const *conf;
> +	void __iomem *regs;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*cm));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, indio_dev);
> +	cm = iio_priv(indio_dev);
> +
> +	conf = of_device_get_match_data(dev);

We try to avoid of specific calls in IIO drivers.
device_get_match_data() should work here.

Reason for this is not that it matters in one driver, but that people tend
to cut and paste device tree stuff and it ends up in drivers where ACPI
and other firmware types are relevant.

Also I only have remember how one type of firmware property read call works ;)

> +	if (!conf) {
> +		dev_err(dev, "failed to match device\n");
> +		return -ENODEV;
> +	}
> +
> +	regs = devm_platform_ioremap_resource_byname(pdev, "reg");
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	cm->reg = devm_regmap_init_mmio(dev, regs, &cmsr_regmap_cfg);
> +	if (IS_ERR(cm->reg)) {
> +		dev_err(dev, "failed to init main regmap: %ld\n",
> +			PTR_ERR(cm->reg));
> +		return PTR_ERR(cm->reg);
> +	}
> +
> +	regs = devm_platform_ioremap_resource_byname(pdev, "duty");
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	cm->duty = devm_regmap_init_mmio(dev, regs, &cmsr_regmap_cfg);
> +	if (IS_ERR(cm->duty)) {
> +		dev_err(dev, "failed to init duty regmap: %ld\n",
> +			PTR_ERR(cm->duty));
> +		return PTR_ERR(cm->duty);
Preference for dev_err_probe() for all errors in probe() and functions
that will only be called form probe. There is a nice version that handled PTR_ERR
under review though so maybe we can come around later and use that once it's available

> +	}
> +
> +	mutex_init(&cm->lock);
> +
> +	/* Init scale with a sane default */
> +	cmsr_set_time_unlocked(cm, CLK_MIN_TIME);
> +
> +	indio_dev->name = "amlogic-clk-msr";
> +	indio_dev->info = &cmsr_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = CLK_MSR_MAX;

Superficially looks like the number of channels depends on the compatible.
Ideally we shouldn't provide channels to userspace that aren't useful.

You could search the name arrays to see how far they go, but that is bit ugly.
Probably wrap those in a structure with a num_channels parameter as well.

> +	indio_dev->channels = cmsr_populate_channels(dev, conf);
> +	if (IS_ERR(indio_dev->channels))
> +		return PTR_ERR(indio_dev->channels);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

[Jerome Brunet]

On Sat 29 Jun 2024 at 20:40, Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 24 Jun 2024 19:31:03 +0200
> Jerome Brunet <jbrunet@baylibre.com> wrote:
>
>> Add support for the HW found in most Amlogic SoC dedicated to measure
>> system clocks.
>> 
>> This drivers aims to replace the one found in
>> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
>> 
>> * Access to the measurements through the IIO API:
>>   Easier re-use of the results in userspace and other drivers
>> * Controllable scale with raw measurements
>> * Higher precision with processed measurements
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> Interesting device and the driver is pretty clean.
>
> Needs a new channel type though as altvoltage is in volts not hz.
>
> Various minor comments inline.
>
> Thanks,

Thanks for the feedback Jonathan.

Just a couple of things,

David expressed concerns with having both IIO_CHAN_INFO_RAW and
IIO_CHAN_INFO_PROCESSED for a channel and we did not reach a conclusion
on this.

My idea here is to:
 * Give full control over the scale to the consumer with
   IIO_CHAN_INFO_RAW
 * Give an easy/convenient way to get an Hz result with
   IIO_CHAN_INFO_PROCESSED. There is a bit more than just 'raw * scale'
   in the implementation of this info to figure out the most appropriate
   scale for the measurement. They idea is also to avoid code
   duplication in consumers.

David is definitely more familiar than me with IIO but we did not really
know how to move forward on this.

Is it OK to have both IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_PROCESSED, or
should I ditch IIO_CHAN_INFO_RAW ?

>
> Jonathan

[...]

>> +	indio_dev->name = "amlogic-clk-msr";
>> +	indio_dev->info = &cmsr_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->num_channels = CLK_MSR_MAX;
>
> Superficially looks like the number of channels depends on the compatible.
> Ideally we shouldn't provide channels to userspace that aren't useful.

Not exactly. All SoCs have 128 inputs, Some may not be connected indeed
but some are, and the name is just not documented (yet).

>
> You could search the name arrays to see how far they go, but that is bit ugly.
> Probably wrap those in a structure with a num_channels parameter as well.
>

I've been doodling with this idea while writing this
driver. Technically, there is no problem.

The legacy driver this one will be replacing used to expose all 128
inputs. I thought it was more important to have continuity with the
legacy driver than filtering out possibly useless channels.

Another benefit of keeping all 128 is that the channel index (both in
sysfs and more crucially in DT) matches the one in the SoC documentation.
That makes things easier.

Would it be acceptable to keep all 128 channels then or do you still
prefer that I filter out the undocumented ones ?

>> +	indio_dev->channels = cmsr_populate_channels(dev, conf);
>> +	if (IS_ERR(indio_dev->channels))
>> +		return PTR_ERR(indio_dev->channels);
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}

Thanks for you help.

-- 
Jerome

Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

[Jonathan Cameron]

On Sun, 30 Jun 2024 09:21:03 +0200
Jerome Brunet <jbrunet@baylibre.com> wrote:

> On Sat 29 Jun 2024 at 20:40, Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Mon, 24 Jun 2024 19:31:03 +0200
> > Jerome Brunet <jbrunet@baylibre.com> wrote:
> >  
> >> Add support for the HW found in most Amlogic SoC dedicated to measure
> >> system clocks.
> >> 
> >> This drivers aims to replace the one found in
> >> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
> >> 
> >> * Access to the measurements through the IIO API:
> >>   Easier re-use of the results in userspace and other drivers
> >> * Controllable scale with raw measurements
> >> * Higher precision with processed measurements
> >> 
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>  
> > Interesting device and the driver is pretty clean.
> >
> > Needs a new channel type though as altvoltage is in volts not hz.
> >
> > Various minor comments inline.
> >
> > Thanks,  
> 
> Thanks for the feedback Jonathan.
> 
> Just a couple of things,
> 
> David expressed concerns with having both IIO_CHAN_INFO_RAW and
> IIO_CHAN_INFO_PROCESSED for a channel and we did not reach a conclusion
> on this.
> 
> My idea here is to:
>  * Give full control over the scale to the consumer with
>    IIO_CHAN_INFO_RAW
>  * Give an easy/convenient way to get an Hz result with
>    IIO_CHAN_INFO_PROCESSED. There is a bit more than just 'raw * scale'
>    in the implementation of this info to figure out the most appropriate
>    scale for the measurement. They idea is also to avoid code
>    duplication in consumers.

So we have had a few cases of software driving autorange finding for sensors
in the past (typically light sensors). An example is the gp2ap020a00f driver but
those have been been driven by interrupts to detect range issue and are much
more complex than what you have here.  Note that driver only provides _RAW,
I think because we have no documentation of how to convert to an illuminance
reading.  It's old though and probably not a good example to follow.

> 
> David is definitely more familiar than me with IIO but we did not really
> know how to move forward on this.
> 
> Is it OK to have both IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_PROCESSED, or
> should I ditch IIO_CHAN_INFO_RAW ?

I'll ask the obvious question - do you have a usecase for _RAW + _SCALE
if not we can take the conservative approach of not providing it 'yet'
and revisit once that use case surfaces.

Otherwise, I tend to resist mixing processed and raw + scale just because
it makes it hard for userspace to understand the difference. This combination
has only happened in the past due to driver evolution (we got it wrong
initially for a given driver). 

So I'm not strongly against providing both (and amending my standard
'don't do this reply' to include this as a special case) but I want to
know someone actually cares.


> 
> >
> > Jonathan  
> 
> [...]
> 
> >> +	indio_dev->name = "amlogic-clk-msr";
> >> +	indio_dev->info = &cmsr_info;
> >> +	indio_dev->modes = INDIO_DIRECT_MODE;
> >> +	indio_dev->num_channels = CLK_MSR_MAX;  
> >
> > Superficially looks like the number of channels depends on the compatible.
> > Ideally we shouldn't provide channels to userspace that aren't useful.  
> 
> Not exactly. All SoCs have 128 inputs, Some may not be connected indeed
> but some are, and the name is just not documented (yet).

> >
> > You could search the name arrays to see how far they go, but that is bit ugly.
> > Probably wrap those in a structure with a num_channels parameter as well.
> >  
> 
> I've been doodling with this idea while writing this
> driver. Technically, there is no problem.
> 
> The legacy driver this one will be replacing used to expose all 128
> inputs. I thought it was more important to have continuity with the
> legacy driver than filtering out possibly useless channels.

ok.
> 
> Another benefit of keeping all 128 is that the channel index (both in
> sysfs and more crucially in DT) matches the one in the SoC documentation.
> That makes things easier.
> 
> Would it be acceptable to keep all 128 channels then or do you still
> prefer that I filter out the undocumented ones ?

Your explanation for why we should keep them is fine.
Add some suitable comments so we don't forget it.

> 
> >> +	indio_dev->channels = cmsr_populate_channels(dev, conf);
> >> +	if (IS_ERR(indio_dev->channels))
> >> +		return PTR_ERR(indio_dev->channels);
> >> +
> >> +	return devm_iio_device_register(dev, indio_dev);
> >> +}  
> 
> Thanks for you help.
> 
You are welcome
J

Re: [PATCH 0/2] iio: frequency: add iio support for Amlogic clock measure

[Neil Armstrong]

Hi,

[+cc people from linux-msm]

On 24/06/2024 19:31, Jerome Brunet wrote:
> Add support for the HW found in most Amlogic SoC dedicated to measure
> system clocks.
> 
> This drivers aims to replace the one found in
> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
> 
> * Access to the measurements through the IIO API:
>    Easier re-use of the results in userspace and other drivers
> * Controllable scale with raw measurements
> * Higher precision with processed measurements
> 
> Jerome Brunet (2):
>    dt-bindings: iio: frequency: add clock measure support
>    iio: frequency: add amlogic clock measure support
> 
>   .../iio/frequency/amlogic,clk-msr-io.yaml     |  50 ++
>   drivers/iio/frequency/Kconfig                 |  15 +
>   drivers/iio/frequency/Makefile                |   1 +
>   drivers/iio/frequency/amlogic-clk-msr-io.c    | 802 ++++++++++++++++++
>   4 files changed, 868 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
>   create mode 100644 drivers/iio/frequency/amlogic-clk-msr-io.c
> 

While I really appreciate the effort, and the code looks cool, the clkmsr is really
a debug tool, and I'm not sure IIO is the right place for such debug tool ?

There's almost the same interface on qcom SoCs (https://github.com/linux-msm/debugcc) but
they chose to keep it in userspace until we find an appropriate way to expose
this from the kernel the right way.

If it enabled us to monitor a frequency input for a product use-case, IIO would be
the appropriate interface, but AFAIK it's only internal clocks and thus I'm worried
it's not the best way to expose those clocks.

Neil

Re: [PATCH 0/2] iio: frequency: add iio support for Amlogic clock measure

[Jerome Brunet]

On Tue 25 Jun 2024 at 11:38, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> Hi,
>
> [+cc people from linux-msm]
>
> On 24/06/2024 19:31, Jerome Brunet wrote:
>> Add support for the HW found in most Amlogic SoC dedicated to measure
>> system clocks.
>> This drivers aims to replace the one found in
>> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
>> * Access to the measurements through the IIO API:
>>    Easier re-use of the results in userspace and other drivers
>> * Controllable scale with raw measurements
>> * Higher precision with processed measurements
>> Jerome Brunet (2):
>>    dt-bindings: iio: frequency: add clock measure support
>>    iio: frequency: add amlogic clock measure support
>>   .../iio/frequency/amlogic,clk-msr-io.yaml     |  50 ++
>>   drivers/iio/frequency/Kconfig                 |  15 +
>>   drivers/iio/frequency/Makefile                |   1 +
>>   drivers/iio/frequency/amlogic-clk-msr-io.c    | 802 ++++++++++++++++++
>>   4 files changed, 868 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
>>   create mode 100644 drivers/iio/frequency/amlogic-clk-msr-io.c
>> 
>
> While I really appreciate the effort, and the code looks cool, the clkmsr is really
> a debug tool, and I'm not sure IIO is the right place for such debug tool ?

The reason why I went through the trouble of doing an IIO port is
because I need that for other purposes than debug. I need to to be able
to check a frequency from another driver. I don't see a reason to invent
another API when IIO provide a perfectly good one.

The HW does measurements. IIO seems like the best place for it.

For the record, I need this for a eARC support.
eARC has a PLL that locks on incoming stream. eARC registers show wether
the PLL is locked or not, but not at which rate. That information is
needed in ASoC. Fortunately the eARC PLL is one of measured clock, which
is a life saver in that case.

Everything that was available through the old driver still is, with more
precision and more control.

>
> There's almost the same interface on qcom SoCs (https://github.com/linux-msm/debugcc) but
> they chose to keep it in userspace until we find an appropriate way to expose
> this from the kernel the right way.
>
> If it enabled us to monitor a frequency input for a product use-case, IIO would be
> the appropriate interface, but AFAIK it's only internal clocks and thus I'm worried
> it's not the best way to expose those clocks.
>
> Neil

-- 
Jerome

Re: [PATCH 0/2] iio: frequency: add iio support for Amlogic clock measure

[Neil Armstrong]

On 25/06/2024 11:53, Jerome Brunet wrote:
> On Tue 25 Jun 2024 at 11:38, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> 
>> Hi,
>>
>> [+cc people from linux-msm]
>>
>> On 24/06/2024 19:31, Jerome Brunet wrote:
>>> Add support for the HW found in most Amlogic SoC dedicated to measure
>>> system clocks.
>>> This drivers aims to replace the one found in
>>> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
>>> * Access to the measurements through the IIO API:
>>>     Easier re-use of the results in userspace and other drivers
>>> * Controllable scale with raw measurements
>>> * Higher precision with processed measurements
>>> Jerome Brunet (2):
>>>     dt-bindings: iio: frequency: add clock measure support
>>>     iio: frequency: add amlogic clock measure support
>>>    .../iio/frequency/amlogic,clk-msr-io.yaml     |  50 ++
>>>    drivers/iio/frequency/Kconfig                 |  15 +
>>>    drivers/iio/frequency/Makefile                |   1 +
>>>    drivers/iio/frequency/amlogic-clk-msr-io.c    | 802 ++++++++++++++++++
>>>    4 files changed, 868 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
>>>    create mode 100644 drivers/iio/frequency/amlogic-clk-msr-io.c
>>>
>>
>> While I really appreciate the effort, and the code looks cool, the clkmsr is really
>> a debug tool, and I'm not sure IIO is the right place for such debug tool ?
> 
> The reason why I went through the trouble of doing an IIO port is
> because I need that for other purposes than debug. I need to to be able
> to check a frequency from another driver. I don't see a reason to invent
> another API when IIO provide a perfectly good one.
> 
> The HW does measurements. IIO seems like the best place for it.
> 
> For the record, I need this for a eARC support.
> eARC has a PLL that locks on incoming stream. eARC registers show wether
> the PLL is locked or not, but not at which rate. That information is
> needed in ASoC. Fortunately the eARC PLL is one of measured clock, which
> is a life saver in that case.

This is a very interesting use-case, and quite weird nothing is provided
on the eARC side.

So yes it's definitely a valid use-case, but:
- we should keep the debugfs interface, perhaps move it in the iio driver ?
- we should keep a single compatible, so simply update the current bindings with iio cells
- for s4 & c3, it's ok to either add a second reg entry in the bindings

Neil

> 
> Everything that was available through the old driver still is, with more
> precision and more control.
> 
>>
>> There's almost the same interface on qcom SoCs (https://github.com/linux-msm/debugcc) but
>> they chose to keep it in userspace until we find an appropriate way to expose
>> this from the kernel the right way.
>>
>> If it enabled us to monitor a frequency input for a product use-case, IIO would be
>> the appropriate interface, but AFAIK it's only internal clocks and thus I'm worried
>> it's not the best way to expose those clocks.
>>
>> Neil
> 

Re: [PATCH 0/2] iio: frequency: add iio support for Amlogic clock measure

[Jerome Brunet]

On Tue 25 Jun 2024 at 15:18, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> On 25/06/2024 11:53, Jerome Brunet wrote:
>> On Tue 25 Jun 2024 at 11:38, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>> 
>>> Hi,
>>>
>>> [+cc people from linux-msm]
>>>
>>> On 24/06/2024 19:31, Jerome Brunet wrote:
>>>> Add support for the HW found in most Amlogic SoC dedicated to measure
>>>> system clocks.
>>>> This drivers aims to replace the one found in
>>>> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
>>>> * Access to the measurements through the IIO API:
>>>>     Easier re-use of the results in userspace and other drivers
>>>> * Controllable scale with raw measurements
>>>> * Higher precision with processed measurements
>>>> Jerome Brunet (2):
>>>>     dt-bindings: iio: frequency: add clock measure support
>>>>     iio: frequency: add amlogic clock measure support
>>>>    .../iio/frequency/amlogic,clk-msr-io.yaml     |  50 ++
>>>>    drivers/iio/frequency/Kconfig                 |  15 +
>>>>    drivers/iio/frequency/Makefile                |   1 +
>>>>    drivers/iio/frequency/amlogic-clk-msr-io.c    | 802 ++++++++++++++++++
>>>>    4 files changed, 868 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
>>>>    create mode 100644 drivers/iio/frequency/amlogic-clk-msr-io.c
>>>>
>>>
>>> While I really appreciate the effort, and the code looks cool, the clkmsr is really
>>> a debug tool, and I'm not sure IIO is the right place for such debug tool ?
>> The reason why I went through the trouble of doing an IIO port is
>> because I need that for other purposes than debug. I need to to be able
>> to check a frequency from another driver. I don't see a reason to invent
>> another API when IIO provide a perfectly good one.
>> The HW does measurements. IIO seems like the best place for it.
>> For the record, I need this for a eARC support.
>> eARC has a PLL that locks on incoming stream. eARC registers show wether
>> the PLL is locked or not, but not at which rate. That information is
>> needed in ASoC. Fortunately the eARC PLL is one of measured clock, which
>> is a life saver in that case.
>
> This is a very interesting use-case, and quite weird nothing is provided
> on the eARC side.

Indeed.

>
> So yes it's definitely a valid use-case, but:
> - we should keep the debugfs interface, perhaps move it in the iio driver ?

I considered this initially but it would add a lot of boiler plate
code to provide over debugfs exactly what iio already provides over
sysfs. As you pointed out, the previous driver only provided debug
information, the debugfs interface it provided is hardly a
critical/stable one.

> - we should keep a single compatible, so simply update the current bindings with iio cells

Using a new compatible allows to split the memory region, making the
interface between DT and driver a lot easier to implement seemlessly
between old and new SoCs. Eventually it may allow to implement the duty
part too.

> - for s4 & c3, it's ok to either add a second reg entry in the bindings

Doing that for s4 and c3 only would still make a mess of offset handling
the region because duty prepend the region on old SoC. The goal is to
have an interface that seemlessly support both old and new SoCs.

>
> Neil
>
>> Everything that was available through the old driver still is, with more
>> precision and more control.
>> 
>>>
>>> There's almost the same interface on qcom SoCs (https://github.com/linux-msm/debugcc) but
>>> they chose to keep it in userspace until we find an appropriate way to expose
>>> this from the kernel the right way.
>>>
>>> If it enabled us to monitor a frequency input for a product use-case, IIO would be
>>> the appropriate interface, but AFAIK it's only internal clocks and thus I'm worried
>>> it's not the best way to expose those clocks.
>>>
>>> Neil
>> 

-- 
Jerome

Re: [PATCH 0/2] iio: frequency: add iio support for Amlogic clock measure

[Neil Armstrong]

On 25/06/2024 15:51, Jerome Brunet wrote:
> On Tue 25 Jun 2024 at 15:18, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> 
>> On 25/06/2024 11:53, Jerome Brunet wrote:
>>> On Tue 25 Jun 2024 at 11:38, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>
>>>> Hi,
>>>>
>>>> [+cc people from linux-msm]
>>>>
>>>> On 24/06/2024 19:31, Jerome Brunet wrote:
>>>>> Add support for the HW found in most Amlogic SoC dedicated to measure
>>>>> system clocks.
>>>>> This drivers aims to replace the one found in
>>>>> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
>>>>> * Access to the measurements through the IIO API:
>>>>>      Easier re-use of the results in userspace and other drivers
>>>>> * Controllable scale with raw measurements
>>>>> * Higher precision with processed measurements
>>>>> Jerome Brunet (2):
>>>>>      dt-bindings: iio: frequency: add clock measure support
>>>>>      iio: frequency: add amlogic clock measure support
>>>>>     .../iio/frequency/amlogic,clk-msr-io.yaml     |  50 ++
>>>>>     drivers/iio/frequency/Kconfig                 |  15 +
>>>>>     drivers/iio/frequency/Makefile                |   1 +
>>>>>     drivers/iio/frequency/amlogic-clk-msr-io.c    | 802 ++++++++++++++++++
>>>>>     4 files changed, 868 insertions(+)
>>>>>     create mode 100644 Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
>>>>>     create mode 100644 drivers/iio/frequency/amlogic-clk-msr-io.c
>>>>>
>>>>
>>>> While I really appreciate the effort, and the code looks cool, the clkmsr is really
>>>> a debug tool, and I'm not sure IIO is the right place for such debug tool ?
>>> The reason why I went through the trouble of doing an IIO port is
>>> because I need that for other purposes than debug. I need to to be able
>>> to check a frequency from another driver. I don't see a reason to invent
>>> another API when IIO provide a perfectly good one.
>>> The HW does measurements. IIO seems like the best place for it.
>>> For the record, I need this for a eARC support.
>>> eARC has a PLL that locks on incoming stream. eARC registers show wether
>>> the PLL is locked or not, but not at which rate. That information is
>>> needed in ASoC. Fortunately the eARC PLL is one of measured clock, which
>>> is a life saver in that case.
>>
>> This is a very interesting use-case, and quite weird nothing is provided
>> on the eARC side.
> 
> Indeed.
> 
>>
>> So yes it's definitely a valid use-case, but:
>> - we should keep the debugfs interface, perhaps move it in the iio driver ?
> 
> I considered this initially but it would add a lot of boiler plate
> code to provide over debugfs exactly what iio already provides over
> sysfs. As you pointed out, the previous driver only provided debug
> information, the debugfs interface it provided is hardly a
> critical/stable one.

I still don't see why it could add so much boilerplate, all the tables and
calculation fonction would be shared, only the debugfs clk_msr_show() and
clk_msr_summary_show() would be kept, all the rest would be common.

I insist, please keep the debugfs interface for debug purposes. You don't
want to mess with IIO when you bring up new platforms with bare minimum
kernels.

> 
>> - we should keep a single compatible, so simply update the current bindings with iio cells
> 
> Using a new compatible allows to split the memory region, making the
> interface between DT and driver a lot easier to implement seemlessly
> between old and new SoCs. Eventually it may allow to implement the duty
> part too.

It's a problem for new platforms, you can introduce the split only for the
new ones, the impact on code won't high enough to justify new bindings.

Neil

> 
>> - for s4 & c3, it's ok to either add a second reg entry in the bindings
> 
> Doing that for s4 and c3 only would still make a mess of offset handling
> the region because duty prepend the region on old SoC. The goal is to
> have an interface that seemlessly support both old and new SoCs.
> 
>>
>> Neil
>>
>>> Everything that was available through the old driver still is, with more
>>> precision and more control.
>>>
>>>>
>>>> There's almost the same interface on qcom SoCs (https://github.com/linux-msm/debugcc) but
>>>> they chose to keep it in userspace until we find an appropriate way to expose
>>>> this from the kernel the right way.
>>>>
>>>> If it enabled us to monitor a frequency input for a product use-case, IIO would be
>>>> the appropriate interface, but AFAIK it's only internal clocks and thus I'm worried
>>>> it's not the best way to expose those clocks.
>>>>
>>>> Neil
>>>
> 

Re: [PATCH 0/2] iio: frequency: add iio support for Amlogic clock measure

[Jerome Brunet]

On Mon 01 Jul 2024 at 09:41, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> On 25/06/2024 15:51, Jerome Brunet wrote:
>> On Tue 25 Jun 2024 at 15:18, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>> 
>>> On 25/06/2024 11:53, Jerome Brunet wrote:
>>>> On Tue 25 Jun 2024 at 11:38, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> [+cc people from linux-msm]
>>>>>
>>>>> On 24/06/2024 19:31, Jerome Brunet wrote:
>>>>>> Add support for the HW found in most Amlogic SoC dedicated to measure
>>>>>> system clocks.
>>>>>> This drivers aims to replace the one found in
>>>>>> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
>>>>>> * Access to the measurements through the IIO API:
>>>>>>      Easier re-use of the results in userspace and other drivers
>>>>>> * Controllable scale with raw measurements
>>>>>> * Higher precision with processed measurements
>>>>>> Jerome Brunet (2):
>>>>>>      dt-bindings: iio: frequency: add clock measure support
>>>>>>      iio: frequency: add amlogic clock measure support
>>>>>>     .../iio/frequency/amlogic,clk-msr-io.yaml     |  50 ++
>>>>>>     drivers/iio/frequency/Kconfig                 |  15 +
>>>>>>     drivers/iio/frequency/Makefile                |   1 +
>>>>>>     drivers/iio/frequency/amlogic-clk-msr-io.c    | 802 ++++++++++++++++++
>>>>>>     4 files changed, 868 insertions(+)
>>>>>>     create mode 100644 Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
>>>>>>     create mode 100644 drivers/iio/frequency/amlogic-clk-msr-io.c
>>>>>>
>>>>>
>>>>> While I really appreciate the effort, and the code looks cool, the clkmsr is really
>>>>> a debug tool, and I'm not sure IIO is the right place for such debug tool ?
>>>> The reason why I went through the trouble of doing an IIO port is
>>>> because I need that for other purposes than debug. I need to to be able
>>>> to check a frequency from another driver. I don't see a reason to invent
>>>> another API when IIO provide a perfectly good one.
>>>> The HW does measurements. IIO seems like the best place for it.
>>>> For the record, I need this for a eARC support.
>>>> eARC has a PLL that locks on incoming stream. eARC registers show wether
>>>> the PLL is locked or not, but not at which rate. That information is
>>>> needed in ASoC. Fortunately the eARC PLL is one of measured clock, which
>>>> is a life saver in that case.
>>>
>>> This is a very interesting use-case, and quite weird nothing is provided
>>> on the eARC side.
>> Indeed.
>> 
>>>
>>> So yes it's definitely a valid use-case, but:
>>> - we should keep the debugfs interface, perhaps move it in the iio driver ?
>> I considered this initially but it would add a lot of boiler plate
>> code to provide over debugfs exactly what iio already provides over
>> sysfs. As you pointed out, the previous driver only provided debug
>> information, the debugfs interface it provided is hardly a
>> critical/stable one.
>
> I still don't see why it could add so much boilerplate, all the tables and
> calculation fonction would be shared, only the debugfs clk_msr_show() and
> clk_msr_summary_show() would be kept, all the rest would be common.
>
> I insist, please keep the debugfs interface for debug purposes. You don't
> want to mess with IIO when you bring up new platforms with bare minimum
> kernels.

I don't think that is going to change anything. It's not like IIO brings
any complexity or will be compiled out.

But since you insist, I'll add it in the next version as a separate patch.

>
>> 
>>> - we should keep a single compatible, so simply update the current bindings with iio cells
>> Using a new compatible allows to split the memory region, making the
>> interface between DT and driver a lot easier to implement seemlessly
>> between old and new SoCs. Eventually it may allow to implement the duty
>> part too.
>
> It's a problem for new platforms, you can introduce the split only for the
> new ones, the impact on code won't high enough to justify new bindings.
>

What you are requesting will introduce two drivers providing the same
compatible, unless you plan on removing the old one in a coordinated
way.

That's an unncessary churn. The old driver could stay there for a
while and platform slowly migrate. What you are requesting forcefully
migrates every consumer, assuming the old driver is compiled out.

This is an opportunity to more correctly describe the interface.
It does not break any DT rules, that is enough of a justification IMO.

> Neil
>
>> 
>>> - for s4 & c3, it's ok to either add a second reg entry in the bindings
>> Doing that for s4 and c3 only would still make a mess of offset handling
>> the region because duty prepend the region on old SoC. The goal is to
>> have an interface that seemlessly support both old and new SoCs.
>> 
>>>
>>> Neil
>>>
>>>> Everything that was available through the old driver still is, with more
>>>> precision and more control.
>>>>
>>>>>
>>>>> There's almost the same interface on qcom SoCs (https://github.com/linux-msm/debugcc) but
>>>>> they chose to keep it in userspace until we find an appropriate way to expose
>>>>> this from the kernel the right way.
>>>>>
>>>>> If it enabled us to monitor a frequency input for a product use-case, IIO would be
>>>>> the appropriate interface, but AFAIK it's only internal clocks and thus I'm worried
>>>>> it's not the best way to expose those clocks.
>>>>>
>>>>> Neil
>>>>
>> 

-- 
Jerome

Re: [PATCH 0/2] iio: frequency: add iio support for Amlogic clock measure

[Neil Armstrong]

On 01/07/2024 11:01, Jerome Brunet wrote:
> On Mon 01 Jul 2024 at 09:41, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> 
>> On 25/06/2024 15:51, Jerome Brunet wrote:
>>> On Tue 25 Jun 2024 at 15:18, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>
>>>> On 25/06/2024 11:53, Jerome Brunet wrote:
>>>>> On Tue 25 Jun 2024 at 11:38, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> [+cc people from linux-msm]
>>>>>>
>>>>>> On 24/06/2024 19:31, Jerome Brunet wrote:
>>>>>>> Add support for the HW found in most Amlogic SoC dedicated to measure
>>>>>>> system clocks.
>>>>>>> This drivers aims to replace the one found in
>>>>>>> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
>>>>>>> * Access to the measurements through the IIO API:
>>>>>>>       Easier re-use of the results in userspace and other drivers
>>>>>>> * Controllable scale with raw measurements
>>>>>>> * Higher precision with processed measurements
>>>>>>> Jerome Brunet (2):
>>>>>>>       dt-bindings: iio: frequency: add clock measure support
>>>>>>>       iio: frequency: add amlogic clock measure support
>>>>>>>      .../iio/frequency/amlogic,clk-msr-io.yaml     |  50 ++
>>>>>>>      drivers/iio/frequency/Kconfig                 |  15 +
>>>>>>>      drivers/iio/frequency/Makefile                |   1 +
>>>>>>>      drivers/iio/frequency/amlogic-clk-msr-io.c    | 802 ++++++++++++++++++
>>>>>>>      4 files changed, 868 insertions(+)
>>>>>>>      create mode 100644 Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
>>>>>>>      create mode 100644 drivers/iio/frequency/amlogic-clk-msr-io.c
>>>>>>>
>>>>>>
>>>>>> While I really appreciate the effort, and the code looks cool, the clkmsr is really
>>>>>> a debug tool, and I'm not sure IIO is the right place for such debug tool ?
>>>>> The reason why I went through the trouble of doing an IIO port is
>>>>> because I need that for other purposes than debug. I need to to be able
>>>>> to check a frequency from another driver. I don't see a reason to invent
>>>>> another API when IIO provide a perfectly good one.
>>>>> The HW does measurements. IIO seems like the best place for it.
>>>>> For the record, I need this for a eARC support.
>>>>> eARC has a PLL that locks on incoming stream. eARC registers show wether
>>>>> the PLL is locked or not, but not at which rate. That information is
>>>>> needed in ASoC. Fortunately the eARC PLL is one of measured clock, which
>>>>> is a life saver in that case.
>>>>
>>>> This is a very interesting use-case, and quite weird nothing is provided
>>>> on the eARC side.
>>> Indeed.
>>>
>>>>
>>>> So yes it's definitely a valid use-case, but:
>>>> - we should keep the debugfs interface, perhaps move it in the iio driver ?
>>> I considered this initially but it would add a lot of boiler plate
>>> code to provide over debugfs exactly what iio already provides over
>>> sysfs. As you pointed out, the previous driver only provided debug
>>> information, the debugfs interface it provided is hardly a
>>> critical/stable one.
>>
>> I still don't see why it could add so much boilerplate, all the tables and
>> calculation fonction would be shared, only the debugfs clk_msr_show() and
>> clk_msr_summary_show() would be kept, all the rest would be common.
>>
>> I insist, please keep the debugfs interface for debug purposes. You don't
>> want to mess with IIO when you bring up new platforms with bare minimum
>> kernels.
> 
> I don't think that is going to change anything. It's not like IIO brings
> any complexity or will be compiled out.
> 
> But since you insist, I'll add it in the next version as a separate patch.
> 
>>
>>>
>>>> - we should keep a single compatible, so simply update the current bindings with iio cells
>>> Using a new compatible allows to split the memory region, making the
>>> interface between DT and driver a lot easier to implement seemlessly
>>> between old and new SoCs. Eventually it may allow to implement the duty
>>> part too.
>>
>> It's a problem for new platforms, you can introduce the split only for the
>> new ones, the impact on code won't high enough to justify new bindings.
>>
> 
> What you are requesting will introduce two drivers providing the same
> compatible, unless you plan on removing the old one in a coordinated
> way.
> 
> That's an unncessary churn. The old driver could stay there for a
> while and platform slowly migrate. What you are requesting forcefully
> migrates every consumer, assuming the old driver is compiled out.
> 
> This is an opportunity to more correctly describe the interface.
> It does not break any DT rules, that is enough of a justification IMO.

DT describes the Hardware, I don't see how the new bindings describes better
the current hardware... tying the new bindings to a new driver is actually
against the DT rules, the bindings thing is actually to avoid that.
For PWM, bindings architecture was clearly wrong, but here, not really.

I still don't see the problem of migrating current users to the new driver
using the current compatible, really, please explain what would be the problem ?

In any case you'll only need to add the #io-channel-cells to boards that would
require frequency monitoring for eARC, for all the other boards you'll won't need it.
So this property can safely be added as optional to the current bindings.

Neil

> 
>> Neil
>>
>>>
>>>> - for s4 & c3, it's ok to either add a second reg entry in the bindings
>>> Doing that for s4 and c3 only would still make a mess of offset handling
>>> the region because duty prepend the region on old SoC. The goal is to
>>> have an interface that seemlessly support both old and new SoCs.
>>>
>>>>
>>>> Neil
>>>>
>>>>> Everything that was available through the old driver still is, with more
>>>>> precision and more control.
>>>>>
>>>>>>
>>>>>> There's almost the same interface on qcom SoCs (https://github.com/linux-msm/debugcc) but
>>>>>> they chose to keep it in userspace until we find an appropriate way to expose
>>>>>> this from the kernel the right way.
>>>>>>
>>>>>> If it enabled us to monitor a frequency input for a product use-case, IIO would be
>>>>>> the appropriate interface, but AFAIK it's only internal clocks and thus I'm worried
>>>>>> it's not the best way to expose those clocks.
>>>>>>
>>>>>> Neil
>>>>>
>>>
>