linux-kernel - Re: [PATCH v5 3/5] clk: spacemit: Add clock support for Spacemit K1 SoC

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <84232793-9cba-4148-9875-d996e85b81be@riscstar.com>
Date: Thu, 20 Mar 2025 17:39:13 -0500
From: Alex Elder <elder@...cstar.com>
To: Haylen Chu <heylenay@....org>, Michael Turquette
 <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Haylen Chu <heylenay@...look.com>,
 Yixun Lan <dlan@...too.org>
Cc: linux-riscv@...ts.infradead.org, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 spacemit@...ts.linux.dev, Inochi Amaoto <inochiama@...look.com>,
 Chen Wang <unicornxdotw@...mail.com>, Jisheng Zhang <jszhang@...nel.org>,
 Meng Zhang <zhangmeng.kevin@...ux.spacemit.com>,
 Guodong Xu <guodong@...cstar.com>
Subject: Re: [PATCH v5 3/5] clk: spacemit: Add clock support for Spacemit K1
 SoC

On 3/11/25 6:19 PM, Alex Elder wrote:
> On 3/6/25 11:57 AM, Haylen Chu wrote:
>> The clock tree of K1 SoC contains three main types of clock hardware
>> (PLL/DDN/MIX) and has control registers split into several multifunction
>> devices: APBS (PLLs), MPMU, APBC and APMU.
>>
>> All register operations are done through regmap to ensure atomiciy
>> between concurrent operations of clock driver and reset,
>> power-domain driver that will be introduced in the future.
>>
>> Signed-off-by: Haylen Chu <heylenay@....org>
> 
> I'm very glad you have the DT issues resolved now.
> 
> I again have lots of comments on the code, and I think I've
> identified a few bugs.  Most of my comments, however, are
> suggesting minor changes for consistency and readability.
> 
> I'm going to skip over a lot of "ccu-k1.c" because most of what I
> say applies to the definitions in the header files.

FYI I encountered a problem I mentioned below.

. . .

>> +/* frequency unit Mhz, return pll vco freq */
>> +static unsigned long ccu_pll_get_vco_freq(struct clk_hw *hw)
>> +{
>> +    const struct ccu_pll_rate_tbl *pll_rate_table;
>> +    struct ccu_pll *p = hw_to_ccu_pll(hw);
>> +    struct ccu_common *common = &p->common;
>> +    u32 swcr1, swcr3, size;
>> +    int i;
>> +
>> +    ccu_read(swcr1, common, &swcr1);
>> +    ccu_read(swcr3, common, &swcr3);
> 
> You are masking off the EN bit, but you should really be
> using a mask defining which bits are valid instead.  As
> I said earlier:
> 
> #define SPACEMIT_PLL_SWCR3_MASK    ~(SPACEMIT_PLL_SWCR3_EN)
> 
>> +    swcr3 &= ~PLL_SWCR3_EN;
> 
>      swcr3 &= SPACEMIT_PLL_SWCR3_MASK;
>> +
>> +    pll_rate_table = p->pll.rate_tbl;
>> +    size = p->pll.tbl_size;
>> +
>> +    for (i = 0; i < size; i++) {
>> +        if (pll_rate_table[i].swcr1 == swcr1 &&
>> +            pll_rate_table[i].swcr3 == swcr3)
>> +            return pll_rate_table[i].rate;
>> +    }
>> +
> 
> I have a general question here.  Once you set one of these
> clock rates, it will always use one of the rates defined
> in the table.
> 
> But what about initially?  Could the hardware start in a
> state that is not defined by this code?  Do you *set* the
> rate initially?  Should you (at least the first time the
> clock is prepared/enabled)?

When doing some testing today I found that the WARN_ON_ONCE()
got called.  I added some information and learned that the
values in hardware of the swcr1 and swcr3 registers were:
   swcr1:  0x0050cd61
   swcr3:  0x3fe00000
I'm not sure which PLL was being used.

So clearly this can happen.  Somehow you need to find a way
to ensure that these registers are initialized to a sane
state (meaning one defined within pll_rate_table[]).

					-Alex

>> +    WARN_ON_ONCE(1);
> 
> Maybe WARN_ONCE(true, "msg");. . .