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

[Jacky Huang <ychuang570808@gmail.com>]

Dear Stephen,


On 2023/3/29 上午 10:19, Stephen Boyd wrote:
> Quoting Jacky Huang (2023-03-28 19:03:24)
>> On 2023/3/29 上午 01:57, Stephen Boyd wrote:
>>> Quoting Jacky Huang (2023-03-27 19:19:08)
>>>> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
>>>> new file mode 100644
>>>> index 000000000000..0740b0b218a7
>>>> --- /dev/null
>>>> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
>>>> @@ -0,0 +1,231 @@
> [...]
>>>> +
>>>> +               L2_0: l2-cache0 {
>>> Just l2-cache for the node name. Doesn't it go under the cpu0 node as
>>> well?
>> This describes the level-2 cache which is external to and shared by cpu0
>> & cpu1.
>> And only level-1 cache is inside of CPU core.
>> L2_0 is must, because both cpu0 and cpu1 has a next-level-cache =
>> <&L2_0> property.
> Ok. The name should just be l2-cache then, not l2-cache0.

OK, I will fix it.

>> Many identical example of l2-cache node can be found in arm64 dts, such
>> as k3-arm642.dtsi,
>> rk3328.dtsi, mt8195.dtsi, etc. Here is just a copy of similar arm64
>> multi-core SoCs.
>>
>> So we would like to keep this unchanged. Is it OK for you? Thanks.
>>
> Mostly ok, yes.

Thank you for the agreement.

>
>>>> +
>>>> +       sys: system-management@40460000 {
>>>> +               compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
>>>> +               reg = <0x0 0x40460000 0x0 0x200>;
>>>> +
>>>> +               reset: reset-controller {
>>>> +                       compatible = "nuvoton,ma35d1-reset";
>>>> +                       #reset-cells = <1>;
>>>> +               };
>>>> +       };
>>>> +
>>>> +       clk: clock-controller@40460200 {
>>>> +               compatible = "nuvoton,ma35d1-clk", "syscon";
>>>> +               reg = <0x00000000 0x40460200 0x0 0x100>;
>>>> +               #clock-cells = <1>;
>>>> +               clocks = <&clk_hxt>;
>>>> +               nuvoton,sys = <&sys>;
>>>> +       };
>>> It looks like the device at 40460000 is a reset and clock controller.
>>> Just make it one node and register the clk or reset device as an
>>> auxiliary device.
>> 40460000 is for system control registers, including power contrl,
>> multifunction pin control,
>> usb phy control, IP reset control, power-on setting information, and
>> many other miscellaneous controls.
>> The registers of reset controller is only a subset of system control
>> registers.
>>
>> 40460200 is for clock controller which is independent of the system
>> control integration
>> The register base of clock controller is very close to system
>> controller, but in fact the two are independent.
> What do you use the syscon for then? The clock driver must want to use
> the syscon for something, implying that they are the same device.

The register lock mechanism is applied to protect many critical 
registers from false written.
The register lock control register is one register in system controller.
Some registers of the clock controller are lock protected. Not only 
clock controller, but other
IP such as RTC, PWM, ADC, etc, also have lock protected registers. All 
these IP requires
syscon to access the lock/unlock control register in the system controller.
That's why we add a <&sys> to the clock controller.

Should we implement a ma35d1-sysctl driver to protect register_lock() 
and register_unlock()
and export to those drivers?  If yes, we can remove the <&sys> from 
clock controller.


Best regards,
Jacky Huang