Messages in this thread | | | From | Martin Blumenstingl <> | Date | Wed, 4 Jul 2018 15:44:09 +0200 | Subject | Re: [PATCH 1/3] soc: amlogic: Add Meson GX Clock Measure driver |
| |
Hi Neil,
On Wed, Jul 4, 2018 at 10:41 AM Neil Armstrong <narmstrong@baylibre.com> wrote: > > Hi Martin, > > On 03/07/2018 21:18, Martin Blumenstingl wrote: > > Hi Neil, > > > > On Tue, Jul 3, 2018 at 3:23 PM Neil Armstrong <narmstrong@baylibre.com> wrote: > >> > >> The Amlogic Meson GX SoCs embeds a clock measurer IP to measure the internal > >> clock paths frequencies. > >> The precision is in the order of the MHz. > >> > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >> --- > >> drivers/soc/amlogic/Kconfig | 8 ++ > >> drivers/soc/amlogic/Makefile | 1 + > >> drivers/soc/amlogic/meson-gx-clk-measure.c | 224 +++++++++++++++++++++++++++++ > >> 3 files changed, 233 insertions(+) > >> create mode 100644 drivers/soc/amlogic/meson-gx-clk-measure.c > >> > >> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig > >> index b04f6e4..4a3217d 100644 > >> --- a/drivers/soc/amlogic/Kconfig > >> +++ b/drivers/soc/amlogic/Kconfig > >> @@ -1,5 +1,13 @@ > >> menu "Amlogic SoC drivers" > >> > >> +config MESON_GX_CLK_MEASURE > >> + bool "Amlogic Meson GX SoC Clock Measure driver" > >> + depends on ARCH_MESON || COMPILE_TEST > >> + default ARCH_MESON > >> + help > >> + Say yes to support of Measuring a set of internal SoC clocks > >> + from the debugfs interface. > >> + > >> config MESON_GX_SOCINFO > >> bool "Amlogic Meson GX SoC Information driver" > >> depends on ARCH_MESON || COMPILE_TEST > >> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile > >> index 8fa3218..a0ad966 100644 > >> --- a/drivers/soc/amlogic/Makefile > >> +++ b/drivers/soc/amlogic/Makefile > >> @@ -1,3 +1,4 @@ > >> +obj-$(CONFIG_MESON_GX_CLK_MEASURE) += meson-gx-clk-measure.o > >> obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o > >> obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o > >> obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o > >> diff --git a/drivers/soc/amlogic/meson-gx-clk-measure.c b/drivers/soc/amlogic/meson-gx-clk-measure.c > >> new file mode 100644 > >> index 0000000..434d9f3 > >> --- /dev/null > >> +++ b/drivers/soc/amlogic/meson-gx-clk-measure.c > > [...] > > >> + CLK_MSR_ID(57, "wave420l_c"), > >> + CLK_MSR_ID(58, "wave420l_b"), > > AFAIK the Chips&Media WAVE420L video codec is only available on GXM (S912) > > I assume reading this on GXBB or GXL simply reads 0? > > Yes > > > > >> + CLK_MSR_ID(59, "hcodec"), > >> + CLK_MSR_ID(60, "alt_32k"), > >> + CLK_MSR_ID(61, "gpio_msr"), > >> + CLK_MSR_ID(62, "hevc"), > >> + CLK_MSR_ID(66, "vid_lock"), > >> + CLK_MSR_ID(70, "pwm_f"), > >> + CLK_MSR_ID(71, "pwm_e"), > >> + CLK_MSR_ID(72, "pwm_d"), > >> + CLK_MSR_ID(73, "pwm_C"), > > should this be pwm_c (instead of pwm_C)? > > > >> + CLK_MSR_ID(75, "aoclkx2_int"), > >> + CLK_MSR_ID(76, "aoclk_int"), > >> + CLK_MSR_ID(77, "rng_ring_osc_0"), > >> + CLK_MSR_ID(78, "rng_ring_osc_1"), > >> + CLK_MSR_ID(79, "rng_ring_osc_2"), > >> + CLK_MSR_ID(80, "rng_ring_osc_3"), > >> + CLK_MSR_ID(81, "vapb"), > >> + CLK_MSR_ID(82, "ge2d"), > >> +}; > > I did a quick check whether the clock IDs are really the same for all GX SoCs: > > apart from clocks missing on older SoCs (see for example the WAVE420L > > clocks above) there were only minor renames but no conflicts! > > I did the same work ! I will add this detail to the commit log. > Thanks for checking ;-) :)
> > > >> + > >> +static int meson_gx_measure_id(struct meson_gx_msr *priv, unsigned int id) > >> +{ > >> + unsigned int val; > >> + int ret; > >> + > >> + regmap_write(priv->regmap, MSR_CLK_REG0, 0); > >> + > >> + /* Set measurement gate to 50uS */ > >> + regmap_update_bits(priv->regmap, MSR_CLK_REG0, MSR_CLK_DIV, > >> + FIELD_PREP(MSR_CLK_DIV, DIV_50US)); > >> + > >> + /* Set ID */ > >> + regmap_update_bits(priv->regmap, MSR_CLK_REG0, MSR_CLK_SRC, > >> + FIELD_PREP(MSR_CLK_SRC, id)); > >> + > >> + /* Enable & Start */ > >> + regmap_update_bits(priv->regmap, MSR_CLK_REG0, > >> + MSR_RUN | MSR_ENABLE, > >> + MSR_RUN | MSR_ENABLE); > >> + > >> + ret = regmap_read_poll_timeout(priv->regmap, MSR_CLK_REG0, > >> + val, !(val & MSR_BUSY), 10, 1000); > >> + if (ret) > >> + return ret; > >> + > >> + /* Disable */ > >> + regmap_update_bits(priv->regmap, MSR_CLK_REG0, MSR_ENABLE, 0); > >> + > >> + /* Get the value in MHz*64 */ > >> + regmap_read(priv->regmap, MSR_CLK_REG2, &val); > >> + > >> + return (((val + 31) & MSR_VAL_MASK) / 64) * 1000000; > >> +} > >> + > >> +static int clk_msr_show(struct seq_file *s, void *data) > >> +{ > >> + struct meson_gx_msr_id *clk_msr_id = s->private; > >> + int val; > >> + > >> + val = meson_gx_measure_id(clk_msr_id->priv, clk_msr_id->id); > >> + if (val < 0) > >> + return val; > >> + > >> + seq_printf(s, "%d\n", val); > > With jerome, we managed to have a much higher precision by cycling over the divider > and checking when the counter overflows. And I will print the precision in the clock debugfs > entry. great that you could even improve the precision!
> >> + > >> + return 0; > >> +} > >> +DEFINE_SHOW_ATTRIBUTE(clk_msr); > > have you considered modelling this as clock driver instead? > > Yes, but it can wait. > Actually this clk-msr can be feed to GEN_CLK and be outputted to a pad, > but all this is only for debug, so we can stick to debugfs for now. > > Question, should I separate the clocks in a subdirectory and add a summary file like > for the clk debugfs ? > > > /sys/kernel/debug/meson-clk-msr > ------------------|--- summary > ------------------|--- clks > -----------------------|--- ring_osc_out_ee_0 > ... > -----------------------|--- ge2d > > ? this is exactly why I was curious if you considered implementing this as clock driver instead based on your description above each measured clock will have the following properties: - id (passed to the clkmsr IP block) - name (human readable name) - clock rate - accuracy
doesn't CCF provide everything except "id"? you'll even get the summary
can you explain how you would use the clkmsr output? here's how I would use it: - assume I'm debugging something broken - I suspect that it may be due to the clock setup - grep "suspicious-clock" /sys/kernel/debug/clk/clk_summary - compare the grep output with /sys/kernel/debug/meson-clk-msr/suspicious-clock
if it was a CCF driver I could simply do: grep "suspicious-clock" /sys/kernel/debug/clk/clk_summary -> this would return "suspicious-clock" (which is what the kernel can configure) and "clkmsr_suspicious-clock" (which is what clkmsr reads)
> > > >> + > >> +static const struct regmap_config clk_msr_regmap_config = { > >> + .reg_bits = 32, > >> + .val_bits = 32, > >> + .reg_stride = 4, > >> + .max_register = MSR_CLK_REG2, > >> +}; > >> + > >> +static int meson_gx_msr_probe(struct platform_device *pdev) > >> +{ > >> + struct meson_gx_msr *priv; > >> + struct resource *res; > >> + struct dentry *root; > >> + void __iomem *base; > >> + int i; > >> + > >> + priv = devm_kzalloc(&pdev->dev, sizeof(struct meson_gx_msr), > >> + GFP_KERNEL); > >> + if (!priv) > >> + return -ENOMEM; > >> + > >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> + base = devm_ioremap_resource(&pdev->dev, res); > >> + if (IS_ERR(base)) { > >> + dev_err(&pdev->dev, "io resource mapping failed\n"); > >> + return PTR_ERR(base); > >> + } > >> + > >> + priv->regmap = devm_regmap_init_mmio(&pdev->dev, base, > >> + &clk_msr_regmap_config); > >> + if (IS_ERR(priv->regmap)) > >> + return PTR_ERR(priv->regmap); > >> + > >> + root = debugfs_create_dir("meson-clk-msr", NULL); > >> + > >> + for (i = 0 ; i < CLK_MSR_MAX ; ++i) { > >> + if (!clk_msr[i].name) > >> + continue; > >> + > >> + clk_msr[i].priv = priv; > >> + > >> + debugfs_create_file(clk_msr[i].name, 0444, root, > >> + &clk_msr[i], &clk_msr_fops); > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static const struct of_device_id meson_gx_msr_match_table[] = { > >> + { .compatible = "amlogic,meson-gx-clk-measure" }, > > maybe pass the meson_gx_msr_id table here because it seems easy to add > > support for AXG and Meson8/Meson8b/Meson8m2 later on > > Yes, I will rename the driver to be generic and pass the table as match data. great, thank you!
Regards Martin
| |