| 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: <3cccf190-112c-47d4-997e-3b3cf5e7a29e@riscstar.com>
Date: Fri, 28 Mar 2025 12:24:31 -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
I realize now I didn't respond to your other comments.
You suggested incorporating this change in the clock patches,
but I believe Haylen wants to avoid doing that (or doesn't want
to use all of it, anyway).
>
> 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?
I think I was trying to shorten things. At this point it's hard
to know which things are K1-specific and which things will be
generic for anything from SpacemiT. Once something new comes
along the code will be updated based on what needs to change.
That said, I'll not rename this, and I will use "spacemit" rather
than "k1" in all of the new symbols I create.
>>
>> 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
That would mean the caller doesn't know about this error.
Why do that? The caller is free to ignore it (but really,
shouldn't). I think it's important to pass this information
back. I'm going to keep this "return ret" in v2.
>
> 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 */ }
I think this is unnecessary but I'll do it--you're the SpacemiT
maintainer so you can define the style you want.
-Alex
>> };
>> --
>> 2.43.0
>>
>
Powered by blists - more mailing lists