[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fe647204-0827-4bf5-8355-80d7e1b6685f@linaro.org>
Date: Wed, 2 Apr 2025 11:22:44 +0200
From: neil.armstrong@...aro.org
To: Chuan Liu <chuan.liu@...ogic.com>, Kevin Hilman <khilman@...libre.com>,
Jerome Brunet <jbrunet@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-amlogic@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] soc: amlogic: clk-measure: Optimize the memory size of
clk-measure
Hi,
On 02/04/2025 10:50, Chuan Liu wrote:
> hi Neil:
>
> Friendly ping: This patch has been approved in review but hasn't been applied yet. Has it been overlooked?😉
Sorry forgot to queue it, doing it right now,
Neil
>
>
> On 2/10/2025 5:50 PM, Neil Armstrong wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On 05/02/2025 08:58, Chuan Liu via B4 Relay wrote:
>>> From: Chuan Liu <chuan.liu@...ogic.com>
>>>
>>> Drop "CLK_MSR_MAX" and replace it with adding a member "msr_count" in
>>> the structure to specify the count of msr_id.
>>>
>>> Mark the table of msr_id as const.
>>>
>>> Signed-off-by: Chuan Liu <chuan.liu@...ogic.com>
>>> ---
>>> The number of msr_ids for each chip is inconsistent. Defining a
>>> fixed-size array for each chip to store msr_ids would waste memory.
>>> ---
>>> Changes in v2:
>>> - Discard the global variable and restore the "priv" member in the
>>> "struct meson_msr_id".
>>> - Mark msr_id_table as const.
>>> - Link to v1: https://lore.kernel.org/r/20250123-optimize_memory_size_of_clk_measure-v1-1-06aa6a01ff37@amlogic.com
>>> ---
>>> Â drivers/soc/amlogic/meson-clk-measure.c | 86 ++++++++++++++++++++++++---------
>>> Â 1 file changed, 62 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
>>> index a6453ffeb753..39638d6a593c 100644
>>> --- a/drivers/soc/amlogic/meson-clk-measure.c
>>> +++ b/drivers/soc/amlogic/meson-clk-measure.c
>>> @@ -33,23 +33,26 @@ static DEFINE_MUTEX(measure_lock);
>>> Â #define DIV_STEPÂ Â Â Â Â Â Â Â Â Â Â 32
>>> Â #define DIV_MAXÂ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 640
>>>
>>> -#define CLK_MSR_MAXÂ Â Â Â Â Â Â Â Â 128
>>> -
>>> Â struct meson_msr_id {
>>> Â Â Â Â Â struct meson_msr *priv;
>>> Â Â Â Â Â unsigned int id;
>>> Â Â Â Â Â const char *name;
>>> Â };
>>>
>>> +struct meson_msr_data {
>>> +Â Â Â Â struct meson_msr_id *msr_table;
>>> +Â Â Â Â unsigned int msr_count;
>>> +};
>>> +
>>> Â struct meson_msr {
>>> Â Â Â Â Â struct regmap *regmap;
>>> -Â Â Â Â struct meson_msr_id msr_table[CLK_MSR_MAX];
>>> +Â Â Â Â struct meson_msr_data data;
>>> Â };
>>>
>>> Â #define CLK_MSR_ID(__id, __name) \
>>> Â Â Â Â Â [__id] = {.id = __id, .name = __name,}
>>>
>>> -static struct meson_msr_id clk_msr_m8[CLK_MSR_MAX] = {
>>> +static const struct meson_msr_id clk_msr_m8[] = {
>>> Â Â Â Â Â CLK_MSR_ID(0, "ring_osc_out_ee0"),
>>> Â Â Â Â Â CLK_MSR_ID(1, "ring_osc_out_ee1"),
>>> Â Â Â Â Â CLK_MSR_ID(2, "ring_osc_out_ee2"),
>>> @@ -98,7 +101,7 @@ static struct meson_msr_id clk_msr_m8[CLK_MSR_MAX] = {
>>> Â Â Â Â Â CLK_MSR_ID(63, "mipi_csi_cfg"),
>>> Â };
>>>
>>> -static struct meson_msr_id clk_msr_gx[CLK_MSR_MAX] = {
>>> +static const struct meson_msr_id clk_msr_gx[] = {
>>> Â Â Â Â Â CLK_MSR_ID(0, "ring_osc_out_ee_0"),
>>> Â Â Â Â Â CLK_MSR_ID(1, "ring_osc_out_ee_1"),
>>> Â Â Â Â Â CLK_MSR_ID(2, "ring_osc_out_ee_2"),
>>> @@ -168,7 +171,7 @@ static struct meson_msr_id clk_msr_gx[CLK_MSR_MAX] = {
>>> Â Â Â Â Â CLK_MSR_ID(82, "ge2d"),
>>> Â };
>>>
>>> -static struct meson_msr_id clk_msr_axg[CLK_MSR_MAX] = {
>>> +static const struct meson_msr_id clk_msr_axg[] = {
>>> Â Â Â Â Â CLK_MSR_ID(0, "ring_osc_out_ee_0"),
>>> Â Â Â Â Â CLK_MSR_ID(1, "ring_osc_out_ee_1"),
>>> Â Â Â Â Â CLK_MSR_ID(2, "ring_osc_out_ee_2"),
>>> @@ -242,7 +245,7 @@ static struct meson_msr_id clk_msr_axg[CLK_MSR_MAX] = {
>>> Â Â Â Â Â CLK_MSR_ID(109, "audio_locker_in"),
>>> Â };
>>>
>>> -static struct meson_msr_id clk_msr_g12a[CLK_MSR_MAX] = {
>>> +static const struct meson_msr_id clk_msr_g12a[] = {
>>> Â Â Â Â Â CLK_MSR_ID(0, "ring_osc_out_ee_0"),
>>> Â Â Â Â Â CLK_MSR_ID(1, "ring_osc_out_ee_1"),
>>> Â Â Â Â Â CLK_MSR_ID(2, "ring_osc_out_ee_2"),
>>> @@ -358,7 +361,7 @@ static struct meson_msr_id clk_msr_g12a[CLK_MSR_MAX] = {
>>> Â Â Â Â Â CLK_MSR_ID(122, "audio_pdm_dclk"),
>>> Â };
>>>
>>> -static struct meson_msr_id clk_msr_sm1[CLK_MSR_MAX] = {
>>> +static const struct meson_msr_id clk_msr_sm1[] = {
>>> Â Â Â Â Â CLK_MSR_ID(0, "ring_osc_out_ee_0"),
>>> Â Â Â Â Â CLK_MSR_ID(1, "ring_osc_out_ee_1"),
>>> Â Â Â Â Â CLK_MSR_ID(2, "ring_osc_out_ee_2"),
>>> @@ -489,7 +492,7 @@ static struct meson_msr_id clk_msr_sm1[CLK_MSR_MAX] = {
>>> Â };
>>>
>>> Â static int meson_measure_id(struct meson_msr_id *clk_msr_id,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â unsigned int duration)
>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â unsigned int duration)
>>> Â {
>>> Â Â Â Â Â struct meson_msr *priv = clk_msr_id->priv;
>>> Â Â Â Â Â unsigned int val;
>>> @@ -573,13 +576,14 @@ DEFINE_SHOW_ATTRIBUTE(clk_msr);
>>> Â static int clk_msr_summary_show(struct seq_file *s, void *data)
>>> Â {
>>> Â Â Â Â Â struct meson_msr_id *msr_table = s->private;
>>> +Â Â Â Â unsigned int msr_count = msr_table->priv->data.msr_count;
>>> Â Â Â Â Â unsigned int precision = 0;
>>> Â Â Â Â Â int val, i;
>>>
>>>      seq_puts(s, " clock                    rate precision\n");
>>> Â Â Â Â Â seq_puts(s, "---------------------------------------------\n");
>>>
>>> -Â Â Â Â for (i = 0 ; i < CLK_MSR_MAX ; ++i) {
>>> +Â Â Â Â for (i = 0 ; i < msr_count ; ++i) {
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â if (!msr_table[i].name)
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â continue;
>>>
>>> @@ -604,7 +608,7 @@ static const struct regmap_config meson_clk_msr_regmap_config = {
>>>
>>> Â static int meson_msr_probe(struct platform_device *pdev)
>>> Â {
>>> -Â Â Â Â const struct meson_msr_id *match_data;
>>> +Â Â Â Â const struct meson_msr_data *match_data;
>>> Â Â Â Â Â struct meson_msr *priv;
>>> Â Â Â Â Â struct dentry *root, *clks;
>>> Â Â Â Â Â void __iomem *base;
>>> @@ -621,7 +625,16 @@ static int meson_msr_probe(struct platform_device *pdev)
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â return -ENODEV;
>>> Â Â Â Â Â }
>>>
>>> -Â Â Â Â memcpy(priv->msr_table, match_data, sizeof(priv->msr_table));
>>> +Â Â Â Â priv->data.msr_table = devm_kcalloc(&pdev->dev,
>>> + match_data->msr_count,
>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â sizeof(struct meson_msr_id),
>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â GFP_KERNEL);
>>> +Â Â Â Â if (!priv->data.msr_table)
>>> +Â Â Â Â Â Â Â Â Â Â Â Â return -ENOMEM;
>>> +
>>> +Â Â Â Â memcpy(priv->data.msr_table, match_data->msr_table,
>>> +Â Â Â Â Â Â Â Â Â Â Â match_data->msr_count * sizeof(struct meson_msr_id));
>>> +Â Â Â Â priv->data.msr_count = match_data->msr_count;
>>>
>>> Â Â Â Â Â base = devm_platform_ioremap_resource(pdev, 0);
>>> Â Â Â Â Â if (IS_ERR(base))
>>> @@ -636,45 +649,70 @@ static int meson_msr_probe(struct platform_device *pdev)
>>> Â Â Â Â Â clks = debugfs_create_dir("clks", root);
>>>
>>> Â Â Â Â Â debugfs_create_file("measure_summary", 0444, root,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â priv->msr_table, &clk_msr_summary_fops);
>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â priv->data.msr_table, &clk_msr_summary_fops);
>>>
>>> -Â Â Â Â for (i = 0 ; i < CLK_MSR_MAX ; ++i) {
>>> -Â Â Â Â Â Â Â Â Â Â Â Â if (!priv->msr_table[i].name)
>>> +Â Â Â Â for (i = 0 ; i < priv->data.msr_count ; ++i) {
>>> +Â Â Â Â Â Â Â Â Â Â Â Â if (!priv->data.msr_table[i].name)
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â continue;
>>>
>>> -Â Â Â Â Â Â Â Â Â Â Â Â priv->msr_table[i].priv = priv;
>>> +Â Â Â Â Â Â Â Â Â Â Â Â priv->data.msr_table[i].priv = priv;
>>>
>>> -Â Â Â Â Â Â Â Â Â Â Â Â debugfs_create_file(priv->msr_table[i].name, 0444, clks,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &priv->msr_table[i], &clk_msr_fops);
>>> + debugfs_create_file(priv->data.msr_table[i].name, 0444, clks,
>>> + &priv->data.msr_table[i], &clk_msr_fops);
>>> Â Â Â Â Â }
>>>
>>> Â Â Â Â Â return 0;
>>> Â }
>>>
>>> +static const struct meson_msr_data clk_msr_gx_data = {
>>> +Â Â Â Â .msr_table = (void *)clk_msr_gx,
>>> +Â Â Â Â .msr_count = ARRAY_SIZE(clk_msr_gx),
>>> +};
>>> +
>>> +static const struct meson_msr_data clk_msr_m8_data = {
>>> +Â Â Â Â .msr_table = (void *)clk_msr_m8,
>>> +Â Â Â Â .msr_count = ARRAY_SIZE(clk_msr_m8),
>>> +};
>>> +
>>> +static const struct meson_msr_data clk_msr_axg_data = {
>>> +Â Â Â Â .msr_table = (void *)clk_msr_axg,
>>> +Â Â Â Â .msr_count = ARRAY_SIZE(clk_msr_axg),
>>> +};
>>> +
>>> +static const struct meson_msr_data clk_msr_g12a_data = {
>>> +Â Â Â Â .msr_table = (void *)clk_msr_g12a,
>>> +Â Â Â Â .msr_count = ARRAY_SIZE(clk_msr_g12a),
>>> +};
>>> +
>>> +static const struct meson_msr_data clk_msr_sm1_data = {
>>> +Â Â Â Â .msr_table = (void *)clk_msr_sm1,
>>> +Â Â Â Â .msr_count = ARRAY_SIZE(clk_msr_sm1),
>>> +};
>>> +
>>> Â static const struct of_device_id meson_msr_match_table[] = {
>>> Â Â Â Â Â {
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â .compatible = "amlogic,meson-gx-clk-measure",
>>> -Â Â Â Â Â Â Â Â Â Â Â Â .data = (void *)clk_msr_gx,
>>> +Â Â Â Â Â Â Â Â Â Â Â Â .data = &clk_msr_gx_data,
>>> Â Â Â Â Â },
>>> Â Â Â Â Â {
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â .compatible = "amlogic,meson8-clk-measure",
>>> -Â Â Â Â Â Â Â Â Â Â Â Â .data = (void *)clk_msr_m8,
>>> +Â Â Â Â Â Â Â Â Â Â Â Â .data = &clk_msr_m8_data,
>>> Â Â Â Â Â },
>>> Â Â Â Â Â {
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â .compatible = "amlogic,meson8b-clk-measure",
>>> -Â Â Â Â Â Â Â Â Â Â Â Â .data = (void *)clk_msr_m8,
>>> +Â Â Â Â Â Â Â Â Â Â Â Â .data = &clk_msr_m8_data,
>>> Â Â Â Â Â },
>>> Â Â Â Â Â {
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â .compatible = "amlogic,meson-axg-clk-measure",
>>> -Â Â Â Â Â Â Â Â Â Â Â Â .data = (void *)clk_msr_axg,
>>> +Â Â Â Â Â Â Â Â Â Â Â Â .data = &clk_msr_axg_data,
>>> Â Â Â Â Â },
>>> Â Â Â Â Â {
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â .compatible = "amlogic,meson-g12a-clk-measure",
>>> -Â Â Â Â Â Â Â Â Â Â Â Â .data = (void *)clk_msr_g12a,
>>> +Â Â Â Â Â Â Â Â Â Â Â Â .data = &clk_msr_g12a_data,
>>> Â Â Â Â Â },
>>> Â Â Â Â Â {
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â .compatible = "amlogic,meson-sm1-clk-measure",
>>> -Â Â Â Â Â Â Â Â Â Â Â Â .data = (void *)clk_msr_sm1,
>>> +Â Â Â Â Â Â Â Â Â Â Â Â .data = &clk_msr_sm1_data,
>>> Â Â Â Â Â },
>>> Â Â Â Â Â { /* sentinel */ }
>>> Â };
>>>
>>> ---
>>> base-commit: 1e1fd26ed4ca05cc1f0e5857918da4dd54967f7d
>>> change-id: 20250123-optimize_memory_size_of_clk_measure-f9c40e815794
>>>
>>> Best regards,
>>
>> Looks fine, thanks !!!
>>
>> Reviewed-by: Neil Armstrong <neil.armstrong@...aro.org>
Powered by blists - more mailing lists