| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <45526855-17b2-4de4-8e12-6320b7d84c8e@riscstar.com>
Date: Sun, 23 Mar 2025 07:43:36 -0500
From: Alex Elder <elder@...cstar.com>
To: Yixun Lan <dlan@...too.org>
Cc: p.zabel@...gutronix.de, mturquette@...libre.com, sboyd@...nel.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, heylenay@....org,
guodong@...cstar.com, paul.walmsley@...ive.com, palmer@...belt.com,
aou@...s.berkeley.edu, spacemit@...ts.linux.dev, devicetree@...r.kernel.org,
linux-clk@...r.kernel.org, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND 2/7] clk: spacemit: define struct k1_ccu_data
On 3/22/25 10:50 AM, Yixun Lan wrote:
> Hi Alex:
>
> this patch change relate to clock only, so how about let's fold
> it into clk patches (which now has not been merged), so we make
> the code right at first place? cause some moving around and renaming
No I don't want to do that.
The clock patches are Haylen's and the are getting closer to
acceptance. Let's not confuse things by adding a bunch of new
functionality. Get those patches in, and mine can follow not
too long after that.
-Alex
>
> On 10:18 Fri 21 Mar , Alex Elder wrote:
>> Define a new structure type to be used for describing the OF match data.
>> Rather than using the array of spacemit_ccu_clk structures for match
>> data, we use this structure instead.
>>
>> Move the definition of the spacemit_ccu_clk structure closer to the top
>> of the source file, and add the new structure definition below it.
>>
>> Shorten the name of spacemit_ccu_register() to be k1_ccu_register().
> any good reason to change this? it make the code style inconsistent,
> do you just change it for shorten function, or want it to be more k1
> specific, so next SoC - e.g maybe k2? will introduce another function?
>
>>
>> Signed-off-by: Alex Elder <elder@...cstar.com>
>> ---
>> drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++---------
>> 1 file changed, 43 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
>> index 44db48ae71313..f7367271396a0 100644
>> --- a/drivers/clk/spacemit/ccu-k1.c
>> +++ b/drivers/clk/spacemit/ccu-k1.c
>> @@ -129,6 +129,15 @@
>> #define APMU_EMAC0_CLK_RES_CTRL 0x3e4
>> #define APMU_EMAC1_CLK_RES_CTRL 0x3ec
>>
>> +struct spacemit_ccu_clk {
>> + int id;
>> + struct clk_hw *hw;
>> +};
>> +
>> +struct k1_ccu_data {
>> + struct spacemit_ccu_clk *clk; /* array with sentinel */
>> +};
>> +
>> /* APBS clocks start */
>>
>> /* Frequency of pll{1,2} should not be updated at runtime */
>> @@ -1359,11 +1368,6 @@ static CCU_GATE_DEFINE(emmc_bus_clk, CCU_PARENT_HW(pmua_aclk),
>> 0);
>> /* APMU clocks end */
>>
>> -struct spacemit_ccu_clk {
>> - int id;
>> - struct clk_hw *hw;
>> -};
>> -
>> static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = {
>> { CLK_PLL1, &pll1.common.hw },
>> { CLK_PLL2, &pll2.common.hw },
>> @@ -1403,6 +1407,10 @@ static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = {
>> { 0, NULL },
>> };
>>
>> +static const struct k1_ccu_data k1_ccu_apbs_data = {
>> + .clk = k1_ccu_apbs_clks,
>> +};
>> +
>> static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = {
>> { CLK_PLL1_307P2, &pll1_d8_307p2.common.hw },
>> { CLK_PLL1_76P8, &pll1_d32_76p8.common.hw },
>> @@ -1440,6 +1448,10 @@ static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = {
>> { 0, NULL },
>> };
>>
>> +static const struct k1_ccu_data k1_ccu_mpmu_data = {
>> + .clk = k1_ccu_mpmu_clks,
>> +};
>> +
>> static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = {
>> { CLK_UART0, &uart0_clk.common.hw },
>> { CLK_UART2, &uart2_clk.common.hw },
>> @@ -1544,6 +1556,10 @@ static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = {
>> { 0, NULL },
>> };
>>
>> +static const struct k1_ccu_data k1_ccu_apbc_data = {
>> + .clk = k1_ccu_apbc_clks,
>> +};
>> +
>> static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = {
>> { CLK_CCI550, &cci550_clk.common.hw },
>> { CLK_CPU_C0_HI, &cpu_c0_hi_clk.common.hw },
>> @@ -1610,9 +1626,13 @@ static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = {
>> { 0, NULL },
>> };
>>
>> -static int spacemit_ccu_register(struct device *dev,
>> - struct regmap *regmap, struct regmap *lock_regmap,
>> - const struct spacemit_ccu_clk *clks)
>> +static const struct k1_ccu_data k1_ccu_apmu_data = {
>> + .clk = k1_ccu_apmu_clks,
>> +};
>> +
>> +static int k1_ccu_register(struct device *dev, struct regmap *regmap,
>> + struct regmap *lock_regmap,
>> + struct spacemit_ccu_clk *clks)
>> {
>> const struct spacemit_ccu_clk *clk;
>> int i, ret, max_id = 0;
>> @@ -1648,15 +1668,24 @@ static int spacemit_ccu_register(struct device *dev,
>>
>> clk_data->num = max_id + 1;
>>
>> - return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
>> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
>> + if (ret)
>> + dev_err(dev, "error %d adding clock hardware provider\n", ret);
>> +
>> + return ret;
> I'd use "return 0;", nothing different, just explicitly short
>
> ok, I can understand this change ease debug procedure once there is problem.
> (but I'm fine with either way, failure should rarely happen & will
> identify early)
>
>> }
>>
>> static int k1_ccu_probe(struct platform_device *pdev)
>> {
>> struct regmap *base_regmap, *lock_regmap = NULL;
>> struct device *dev = &pdev->dev;
>> + const struct k1_ccu_data *data;
>> int ret;
>>
>> + data = of_device_get_match_data(dev);
>> + if (!data)
>> + return -EINVAL;
>> +
>> base_regmap = device_node_to_regmap(dev->of_node);
>> if (IS_ERR(base_regmap))
>> return dev_err_probe(dev, PTR_ERR(base_regmap),
>> @@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev)
>> "failed to get lock regmap\n");
>> }
>>
>> - ret = spacemit_ccu_register(dev, base_regmap, lock_regmap,
>> - of_device_get_match_data(dev));
>> + ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk);
>> if (ret)
>> return dev_err_probe(dev, ret, "failed to register clocks\n");
>>
>> @@ -1688,19 +1716,19 @@ static int k1_ccu_probe(struct platform_device *pdev)
>> static const struct of_device_id of_k1_ccu_match[] = {
>> {
>> .compatible = "spacemit,k1-pll",
>> - .data = k1_ccu_apbs_clks,
>> + .data = &k1_ccu_apbs_data,
>> },
>> {
>> .compatible = "spacemit,k1-syscon-mpmu",
>> - .data = k1_ccu_mpmu_clks,
>> + .data = &k1_ccu_mpmu_data,
>> },
>> {
>> .compatible = "spacemit,k1-syscon-apbc",
>> - .data = k1_ccu_apbc_clks,
>> + .data = &k1_ccu_apbc_data,
>> },
>> {
>> .compatible = "spacemit,k1-syscon-apmu",
>> - .data = k1_ccu_apmu_clks,
>> + .data = &k1_ccu_apmu_data,
>> },
>> { }
> { /* sentinel */ }
>> };
>> --
>> 2.43.0
>>
>
Powered by blists - more mailing lists