Hi Neil,


On 4/14/2025 6:21 PM, Neil Armstrong wrote:

[ EXTERNAL EMAIL ]

On 14/04/2025 12:12, Chuan Liu via B4 Relay wrote:

quoted

From: Chuan Liu <redacted>

Since the MSR_CLK register offset differs between chip variants, we
replace the macro-based definition with chip-specific assignments.

Change the max_register in regmap_config to be retrieved from DTS.

Signed-off-by: Chuan Liu <redacted>
---
  drivers/soc/amlogic/meson-clk-measure.c | 54 
++++++++++++++++++++++++---------
  1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/amlogic/meson-clk-measure.c 

b/drivers/soc/amlogic/meson-clk-measure.c
index 39638d6a593c..82c008ade894 100644

--- a/drivers/soc/amlogic/meson-clk-measure.c
+++ b/drivers/soc/amlogic/meson-clk-measure.c

@@ -14,11 +14,6 @@

  static DEFINE_MUTEX(measure_lock);

-#define MSR_CLK_DUTY         0x0
-#define MSR_CLK_REG0         0x4
-#define MSR_CLK_REG1         0x8
-#define MSR_CLK_REG2         0xc
-
  #define MSR_DURATION                GENMASK(15, 0)
  #define MSR_ENABLE          BIT(16)
  #define MSR_CONT            BIT(17) /* continuous measurement */

@@ -39,9 +34,17 @@ struct meson_msr_id {

      const char *name;
  };

+struct msr_reg_offset {
+     unsigned int duty_val;
+     unsigned int freq_ctrl;
+     unsigned int duty_ctrl;
+     unsigned int freq_val;
+};
+
  struct meson_msr_data {
      struct meson_msr_id *msr_table;
      unsigned int msr_count;
+     struct msr_reg_offset *reg;

const truct msr_reg_offset *reg;

quoted

  };

  struct meson_msr {

@@ -495,6 +498,7 @@ static int meson_measure_id(struct meson_msr_id 

*clk_msr_id,
                          unsigned int duration)
  {
      struct meson_msr *priv = clk_msr_id->priv;
+     struct msr_reg_offset *reg = priv->data.reg;
      unsigned int val;
      int ret;

@@ -502,22 +506,22 @@ static int meson_measure_id(struct meson_msr_id 

*clk_msr_id,
      if (ret)
              return ret;

-     regmap_write(priv->regmap, MSR_CLK_REG0, 0);
+     regmap_write(priv->regmap, reg->freq_ctrl, 0);

      /* Set measurement duration */
-     regmap_update_bits(priv->regmap, MSR_CLK_REG0, MSR_DURATION,
+     regmap_update_bits(priv->regmap, reg->freq_ctrl, MSR_DURATION,
                         FIELD_PREP(MSR_DURATION, duration - 1));

      /* Set ID */
-     regmap_update_bits(priv->regmap, MSR_CLK_REG0, MSR_CLK_SRC,
+     regmap_update_bits(priv->regmap, reg->freq_ctrl, MSR_CLK_SRC,
                         FIELD_PREP(MSR_CLK_SRC, clk_msr_id->id));

      /* Enable & Start */
-     regmap_update_bits(priv->regmap, MSR_CLK_REG0,
+     regmap_update_bits(priv->regmap, reg->freq_ctrl,
                         MSR_RUN | MSR_ENABLE,
                         MSR_RUN | MSR_ENABLE);

-     ret = regmap_read_poll_timeout(priv->regmap, MSR_CLK_REG0,
+     ret = regmap_read_poll_timeout(priv->regmap, reg->freq_ctrl,
                                     val, !(val & MSR_BUSY), 10, 10000);
      if (ret) {
              mutex_unlock(&measure_lock);

@@ -525,10 +529,10 @@ static int meson_measure_id(struct meson_msr_id 

*clk_msr_id,
      }

      /* Disable */
-     regmap_update_bits(priv->regmap, MSR_CLK_REG0, MSR_ENABLE, 0);
+     regmap_update_bits(priv->regmap, reg->freq_ctrl, MSR_ENABLE, 0);

      /* Get the value in multiple of gate time counts */
-     regmap_read(priv->regmap, MSR_CLK_REG2, &val);
+     regmap_read(priv->regmap, reg->freq_val, &val);

      mutex_unlock(&measure_lock);

@@ -599,11 +603,10 @@ static int clk_msr_summary_show(struct seq_file 

*s, void *data)
  }
  DEFINE_SHOW_ATTRIBUTE(clk_msr_summary);

-static const struct regmap_config meson_clk_msr_regmap_config = {
+static struct regmap_config meson_clk_msr_regmap_config = {
      .reg_bits = 32,
      .val_bits = 32,
      .reg_stride = 4,
-     .max_register = MSR_CLK_REG2,
  };

  static int meson_msr_probe(struct platform_device *pdev)

@@ -611,6 +614,7 @@ static int meson_msr_probe(struct platform_device 

*pdev)
      const struct meson_msr_data *match_data;
      struct meson_msr *priv;
      struct dentry *root, *clks;
+     struct resource *res;
      void __iomem *base;
      int i;

@@ -636,15 +640,23 @@ static int meson_msr_probe(struct 

platform_device *pdev)
             match_data->msr_count * sizeof(struct meson_msr_id));
      priv->data.msr_count = match_data->msr_count;

-     base = devm_platform_ioremap_resource(pdev, 0);
+     base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
      if (IS_ERR(base))
              return PTR_ERR(base);

+     meson_clk_msr_regmap_config.max_register = resource_size(res) - 4;
      priv->regmap = devm_regmap_init_mmio(&pdev->dev, base,
&meson_clk_msr_regmap_config);
      if (IS_ERR(priv->regmap))
              return PTR_ERR(priv->regmap);

+     priv->data.reg = devm_kzalloc(&pdev->dev, sizeof(struct 
msr_reg_offset),
+                                   GFP_KERNEL);
+     if (!priv->data.reg)
+             return -ENOMEM;
+
+     memcpy(priv->data.reg, match_data->reg, sizeof(struct 
msr_reg_offset));

If define struct msr_reg_offset as const, it causes a compilation error
here. What's the better way to handle this?
1) Forced type conversion:
     memcpy((void*)priv->data.reg, match_data->reg,
                    sizeof(struct msr_reg_offset));
2) Directly assign the already-defined meson_msr_data->reg (Remove
dynamic memory allocation for 'priv->data.reg'"):
     priv->data.reg = match_data->reg;

quoted

+
      root = debugfs_create_dir("meson-clk-msr", NULL);
      clks = debugfs_create_dir("clks", root);

@@ -664,29 +676,41 @@ static int meson_msr_probe(struct 

platform_device *pdev)
      return 0;
  }

+struct msr_reg_offset msr_reg_offset = {
+     .duty_val = 0x0,
+     .freq_ctrl = 0x4,
+     .duty_ctrl = 0x8,
+     .freq_val = 0xc,
+};

This should be static const

quoted

+
  static const struct meson_msr_data clk_msr_gx_data = {
      .msr_table = (void *)clk_msr_gx,
      .msr_count = ARRAY_SIZE(clk_msr_gx),
+     .reg = &msr_reg_offset,
  };

  static const struct meson_msr_data clk_msr_m8_data = {
      .msr_table = (void *)clk_msr_m8,
      .msr_count = ARRAY_SIZE(clk_msr_m8),
+     .reg = &msr_reg_offset,
  };

  static const struct meson_msr_data clk_msr_axg_data = {
      .msr_table = (void *)clk_msr_axg,
      .msr_count = ARRAY_SIZE(clk_msr_axg),
+     .reg = &msr_reg_offset,
  };

  static const struct meson_msr_data clk_msr_g12a_data = {
      .msr_table = (void *)clk_msr_g12a,
      .msr_count = ARRAY_SIZE(clk_msr_g12a),
+     .reg = &msr_reg_offset,
  };

  static const struct meson_msr_data clk_msr_sm1_data = {
      .msr_table = (void *)clk_msr_sm1,
      .msr_count = ARRAY_SIZE(clk_msr_sm1),
+     .reg = &msr_reg_offset,
  };

  static const struct of_device_id meson_msr_match_table[] = {

With this fixed:

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Thanks,
Neil