Re: [PATCH v6 08/12] arm64: dts: nuvoton: Add initial ma35d1 device tree

[Jacky Huang <ychuang570808@gmail.com>]

Dear Stephen,



On 2023/3/29 上午 11:54, Stephen Boyd wrote:
> Quoting Jacky Huang (2023-03-28 20:43:23)
>> On 2023/3/29 上午 11:25, Stephen Boyd wrote:
>>> Quoting Jacky Huang (2023-03-28 20:13:11)
>>>> I may not explain clearly enough. The lock/unlock register of system
>>>> controller is more like
>>>> a kind of write protection for specific registers, rather than
>>>> preventing hetero-core CPU access.
>>>> In many different IP of ma35d1 contain write protected registers.
>>>> In fact, ma35d1 has a "hardware semaphore" IP, and we have implemented
>>>> the driver in drivers/hwspinlock.
>>>> Even the control register of "hardware semaphore" is also write protected.
>>> What's the need to lock and unlock the registers? Is some other
>>> processor also writing to the registers that we need to synchronize
>>> against? Or is Linux the only entity reading and writing the registers?
>>> I'm wondering if we should simply unlock the registers and never lock
>>> them.
> Can you answer this question?

Sorry, I miss this.
The lock and unlock register mechanism is a hardware design of ma35d1 SoC.
The purpose is to prevent from unexpected write to some registers.
However, I think this is a redundant design if s/w is done properly.
Even though I think it's a redundant design, it's out there and we have 
to deal with it.
And of course we have unlock and lock pair, I just lost to write in the 
above.

>>>> So, should we implement a system controller driver to provide
>>>> register_unlock() function?
>>>> Is it OK to have such a driver in drivers/mfd?
>>>> Or, just use syscon in device tree for those devices that have write
>>>> protect registers.
>>>>
>>> The hwspinlock framework doesn't require there to be another entity
>>> accessing some resource. It's there to implement hardware locks. I don't
>>> see why it can't be used here.
>> The current usage of register lock/unlock protect is as the following code:
>>
>> static void ma35d1_unlock_regs(struct ma35d1_clk_pll *pll)
>> {
>>       int ret;
>>
>>       do {
>>           regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x59);
>>           regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x16);
>>           regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x88);
>>           regmap_read(pll->regmap, REG_SYS_RLKTZNS, &ret);
>>       } while (ret == 0);
>> }
>>
>> static void ma35d1_lock_regs(struct ma35d1_clk_pll *pll)
>> {
>>       regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x0);
>> }
>>
>> And the following code is to unlock registers for write and then lock again.
>>
>>       ma35d1_unlock_regs(pll);
>>       writel_relaxed(reg_ctl[0], pll->ctl0_base);
>>       writel_relaxed(reg_ctl[1], pll->ctl1_base);
>>       writel_relaxed(reg_ctl[2], pll->ctl2_base);
>>       ma35d1_lock_regs(pll);
>>
>> The above code is from the clk-ma35d1-pll.c from this patchset.
> Yeah I understand that you write some registers in the syscon to lock
> the registers.
>
>> We just employ regmap mechansim for the access to REG_SYS_RLKTZNS register.
>> Is this implementation OK for you?  Thank you.
>>
> No. Why can't that be a hwspinlock? Or why can't it be unlocked all the
> time and rely on software spinlocks in the kernel to prevent concurrent
> access to the registers accessed by a driver, like a lock for the clk
> registers and a lock for the reset registers, etc. Or if no two clks or
> resets exist within one 32-bit word then no lock is necessary.

You gave a good suggestion here. Yes, of course we can let it be 
unlocked all the time.
We can unlock the "register lock" before entering Linux.
As a result, the unlonk and lock register related code is not required.
Thus, we can remove syscon from the clock controller node.

If you agree with this, we will modify it in the next version.


Best regards,
Jacky Huang