[v6,08/12] arm64: dts: nuvoton: Add initial ma35d1 device tree
Commit Message
From: Jacky Huang <ychuang3@nuvoton.com>
Add initial device tree support for Nuvoton ma35d1 SoC, including
cpu, clock, reset, and serial controllers.
Add reference boards som-256m and iot-512m.
Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
---
arch/arm64/boot/dts/nuvoton/Makefile | 2 +
.../boot/dts/nuvoton/ma35d1-iot-512m.dts | 56 +++++
.../boot/dts/nuvoton/ma35d1-som-256m.dts | 56 +++++
arch/arm64/boot/dts/nuvoton/ma35d1.dtsi | 231 ++++++++++++++++++
4 files changed, 345 insertions(+)
create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
Comments
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 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Nuvoton Technology Corp.
> + * Author: Shan-Chun Hung <schung@nuvoton.com>
> + * Jacky huang <ychuang3@nuvoton.com>
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
> +#include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
> +
> +/ {
> + compatible = "nuvoton,ma35d1";
> + interrupt-parent = <&gic>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + cpus {
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + cpu0: cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a35";
> + reg = <0x0 0x0>;
> + enable-method = "psci";
> + next-level-cache = <&L2_0>;
> + };
> +
> + cpu1: cpu@1 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a35";
> + reg = <0x0 0x1>;
> + enable-method = "psci";
> + next-level-cache = <&L2_0>;
> + };
> +
> + L2_0: l2-cache0 {
Just l2-cache for the node name. Doesn't it go under the cpu0 node as
well?
> + compatible = "cache";
> + cache-level = <2>;
> + };
> + };
> +
> + psci {
> + compatible = "arm,psci-0.2";
> + method = "smc";
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
> + IRQ_TYPE_LEVEL_LOW)>, /* Physical Secure */
> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
> + IRQ_TYPE_LEVEL_LOW)>, /* Physical Non-Secure */
> + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
> + IRQ_TYPE_LEVEL_LOW)>, /* Virtual */
> + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
> + IRQ_TYPE_LEVEL_LOW)>; /* Hypervisor */
> + clock-frequency = <12000000>;
Remove this property. The frequency should be read by the driver.
> + interrupt-parent = <&gic>;
> + };
Please create an 'soc' node for the SoC to hold all the nodes that have
a reg property.
> +
> + 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.
> +
> + gic: interrupt-controller@50801000 {
> + compatible = "arm,gic-400";
> + reg = <0x0 0x50801000 0 0x1000>, /* GICD */
> + <0x0 0x50802000 0 0x2000>, /* GICC */
> + <0x0 0x50804000 0 0x2000>, /* GICH */
> + <0x0 0x50806000 0 0x2000>; /* GICV */
> + #interrupt-cells = <3>;
> + interrupt-parent = <&gic>;
> + interrupt-controller;
> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) |
> + IRQ_TYPE_LEVEL_HIGH)>;
> + };
> +
> + uart0:serial@40700000 {
Add a space after :
> + compatible = "nuvoton,ma35d1-uart";
> + reg = <0x0 0x40700000 0x0 0x100>;
> + interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clk UART0_GATE>;
> + status = "disabled";
> + };
Dear Stephen,
Thanks for your advice.
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 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023 Nuvoton Technology Corp.
>> + * Author: Shan-Chun Hung <schung@nuvoton.com>
>> + * Jacky huang <ychuang3@nuvoton.com>
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
>> +#include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
>> +
>> +/ {
>> + compatible = "nuvoton,ma35d1";
>> + interrupt-parent = <&gic>;
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> +
>> + cpus {
>> + #address-cells = <2>;
>> + #size-cells = <0>;
>> +
>> + cpu0: cpu@0 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a35";
>> + reg = <0x0 0x0>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_0>;
>> + };
>> +
>> + cpu1: cpu@1 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a35";
>> + reg = <0x0 0x1>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_0>;
>> + };
>> +
>> + 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.
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.
>> + compatible = "cache";
>> + cache-level = <2>;
>> + };
>> + };
>> +
>> + psci {
>> + compatible = "arm,psci-0.2";
>> + method = "smc";
>> + };
>> +
>> + timer {
>> + compatible = "arm,armv8-timer";
>> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
>> + IRQ_TYPE_LEVEL_LOW)>, /* Physical Secure */
>> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
>> + IRQ_TYPE_LEVEL_LOW)>, /* Physical Non-Secure */
>> + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
>> + IRQ_TYPE_LEVEL_LOW)>, /* Virtual */
>> + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
>> + IRQ_TYPE_LEVEL_LOW)>; /* Hypervisor */
>> + clock-frequency = <12000000>;
> Remove this property. The frequency should be read by the driver.
I will remove it.
>> + interrupt-parent = <&gic>;
>> + };
> Please create an 'soc' node for the SoC to hold all the nodes that have
> a reg property.
OK, we will use soc node in the next version.
>> +
>> + 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.
>> +
>> + gic: interrupt-controller@50801000 {
>> + compatible = "arm,gic-400";
>> + reg = <0x0 0x50801000 0 0x1000>, /* GICD */
>> + <0x0 0x50802000 0 0x2000>, /* GICC */
>> + <0x0 0x50804000 0 0x2000>, /* GICH */
>> + <0x0 0x50806000 0 0x2000>; /* GICV */
>> + #interrupt-cells = <3>;
>> + interrupt-parent = <&gic>;
>> + interrupt-controller;
>> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) |
>> + IRQ_TYPE_LEVEL_HIGH)>;
>> + };
>> +
>> + uart0:serial@40700000 {
> Add a space after :
I will fix it. Thank you.
>> + compatible = "nuvoton,ma35d1-uart";
>> + reg = <0x0 0x40700000 0x0 0x100>;
>> + interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&clk UART0_GATE>;
>> + status = "disabled";
>> + };
Best regards,
Jacky Huang
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.
>
> 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.
>
> >> +
> >> + 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.
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
Quoting Jacky Huang (2023-03-28 19:39:36)
> On 2023/3/29 上午 10:19, Stephen Boyd wrote:
> > 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.
>
You can implement the lock and unlock in the hwspinlock framework. See
drivers/hwspinlock.
Dear Stephen,
On 2023/3/29 上午 10:46, Stephen Boyd wrote:
> Quoting Jacky Huang (2023-03-28 19:39:36)
>> On 2023/3/29 上午 10:19, Stephen Boyd wrote:
>>> 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.
>>
> You can implement the lock and unlock in the hwspinlock framework. See
> drivers/hwspinlock.
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.
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.
Best regards,
Jacky Huang
Quoting Jacky Huang (2023-03-28 20:13:11)
> Dear Stephen,
>
>
> On 2023/3/29 上午 10:46, Stephen Boyd wrote:
> > Quoting Jacky Huang (2023-03-28 19:39:36)
> >> On 2023/3/29 上午 10:19, Stephen Boyd wrote:
> >>> 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.
> >>
> > You can implement the lock and unlock in the hwspinlock framework. See
> > drivers/hwspinlock.
>
> 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.
>
> 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.
Dear Stephen,
On 2023/3/29 上午 11:25, Stephen Boyd wrote:
> Quoting Jacky Huang (2023-03-28 20:13:11)
>> Dear Stephen,
>>
>>
>> On 2023/3/29 上午 10:46, Stephen Boyd wrote:
>>> Quoting Jacky Huang (2023-03-28 19:39:36)
>>>> On 2023/3/29 上午 10:19, Stephen Boyd wrote:
>>>>> 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.
>>>>
>>> You can implement the lock and unlock in the hwspinlock framework. See
>>> drivers/hwspinlock.
>> 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.
>
>> 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.
We just employ regmap mechansim for the access to REG_SYS_RLKTZNS register.
Is this implementation OK for you? Thank you.
Best regards,
Jacky Huang
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?
> >
> >> 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.
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
On 28/03/2023 04:19, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
>
> Add initial device tree support for Nuvoton ma35d1 SoC, including
> cpu, clock, reset, and serial controllers.
> Add reference boards som-256m and iot-512m.
>
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> + gic: interrupt-controller@50801000 {
> + compatible = "arm,gic-400";
> + reg = <0x0 0x50801000 0 0x1000>, /* GICD */
> + <0x0 0x50802000 0 0x2000>, /* GICC */
> + <0x0 0x50804000 0 0x2000>, /* GICH */
> + <0x0 0x50806000 0 0x2000>; /* GICV */
> + #interrupt-cells = <3>;
> + interrupt-parent = <&gic>;
> + interrupt-controller;
> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) |
> + IRQ_TYPE_LEVEL_HIGH)>;
> + };
> +
> + uart0:serial@40700000 {
There is always space after label:.
Best regards,
Krzysztof
Dear Krzysztof,
On 2023/3/29 下午 04:21, Krzysztof Kozlowski wrote:
> On 28/03/2023 04:19, Jacky Huang wrote:
>> From: Jacky Huang <ychuang3@nuvoton.com>
>>
>> Add initial device tree support for Nuvoton ma35d1 SoC, including
>> cpu, clock, reset, and serial controllers.
>> Add reference boards som-256m and iot-512m.
>>
>> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
>
>
>> + gic: interrupt-controller@50801000 {
>> + compatible = "arm,gic-400";
>> + reg = <0x0 0x50801000 0 0x1000>, /* GICD */
>> + <0x0 0x50802000 0 0x2000>, /* GICC */
>> + <0x0 0x50804000 0 0x2000>, /* GICH */
>> + <0x0 0x50806000 0 0x2000>; /* GICV */
>> + #interrupt-cells = <3>;
>> + interrupt-parent = <&gic>;
>> + interrupt-controller;
>> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) |
>> + IRQ_TYPE_LEVEL_HIGH)>;
>> + };
>> +
>> + uart0:serial@40700000 {
> There is always space after label:.
>
>
> Best regards,
> Krzysztof
>
I will fix them all.
Best regards,
Jacky Huang
Quoting Jacky Huang (2023-03-28 23:02:31)
>
> On 2023/3/29 上午 11:54, Stephen Boyd wrote:
> > Quoting Jacky Huang (2023-03-28 20:43:23)
> >
> >> 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.
>
Sure, that works.
@@ -1,2 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
dtb-$(CONFIG_ARCH_NPCM) += nuvoton-npcm845-evb.dtb
+dtb-$(CONFIG_ARCH_NUVOTON) += ma35d1-iot-512m.dtb
+dtb-$(CONFIG_ARCH_NUVOTON) += ma35d1-som-256m.dtb
new file mode 100644
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ * Author: Shan-Chun Hung <schung@nuvoton.com>
+ * Jacky huang <ychuang3@nuvoton.com>
+ */
+
+/dts-v1/;
+#include "ma35d1.dtsi"
+
+/ {
+ model = "Nuvoton MA35D1-IoT";
+ compatible = "nuvoton,ma35d1-iot", "nuvoton,ma35d1";
+
+ aliases {
+ serial0 = &uart0;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ mem: memory@80000000 {
+ device_type = "memory";
+ reg = <0x00000000 0x80000000 0 0x20000000>; /* 512M DRAM */
+ };
+
+ clk_hxt: clock-hxt {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <24000000>;
+ clock-output-names = "clk_hxt";
+ };
+};
+
+&uart0 {
+ status = "okay";
+};
+
+&clk {
+ assigned-clocks = <&clk CAPLL>,
+ <&clk DDRPLL>,
+ <&clk APLL>,
+ <&clk EPLL>,
+ <&clk VPLL>;
+ assigned-clock-rates = <800000000>,
+ <266000000>,
+ <180000000>,
+ <500000000>,
+ <102000000>;
+ nuvoton,pll-mode = "integer",
+ "fractional",
+ "integer",
+ "integer",
+ "integer";
+};
new file mode 100644
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ * Author: Shan-Chun Hung <schung@nuvoton.com>
+ * Jacky huang <ychuang3@nuvoton.com>
+ */
+
+/dts-v1/;
+#include "ma35d1.dtsi"
+
+/ {
+ model = "Nuvoton MA35D1-SOM";
+ compatible = "nuvoton,ma35d1-som", "nuvoton,ma35d1";
+
+ aliases {
+ serial0 = &uart0;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ mem: memory@80000000 {
+ device_type = "memory";
+ reg = <0x00000000 0x80000000 0 0x10000000>; /* 256M DRAM */
+ };
+
+ clk_hxt: clock-hxt {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <24000000>;
+ clock-output-names = "clk_hxt";
+ };
+};
+
+&uart0 {
+ status = "okay";
+};
+
+&clk {
+ assigned-clocks = <&clk CAPLL>,
+ <&clk DDRPLL>,
+ <&clk APLL>,
+ <&clk EPLL>,
+ <&clk VPLL>;
+ assigned-clock-rates = <800000000>,
+ <266000000>,
+ <180000000>,
+ <500000000>,
+ <102000000>;
+ nuvoton,pll-mode = "integer",
+ "fractional",
+ "integer",
+ "integer",
+ "integer";
+};
new file mode 100644
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ * Author: Shan-Chun Hung <schung@nuvoton.com>
+ * Jacky huang <ychuang3@nuvoton.com>
+ */
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+#include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
+
+/ {
+ compatible = "nuvoton,ma35d1";
+ interrupt-parent = <&gic>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ cpus {
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ cpu0: cpu@0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ reg = <0x0 0x0>;
+ enable-method = "psci";
+ next-level-cache = <&L2_0>;
+ };
+
+ cpu1: cpu@1 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a35";
+ reg = <0x0 0x1>;
+ enable-method = "psci";
+ next-level-cache = <&L2_0>;
+ };
+
+ L2_0: l2-cache0 {
+ compatible = "cache";
+ cache-level = <2>;
+ };
+ };
+
+ psci {
+ compatible = "arm,psci-0.2";
+ method = "smc";
+ };
+
+ timer {
+ compatible = "arm,armv8-timer";
+ interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>, /* Physical Secure */
+ <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>, /* Physical Non-Secure */
+ <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>, /* Virtual */
+ <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>; /* Hypervisor */
+ clock-frequency = <12000000>;
+ interrupt-parent = <&gic>;
+ };
+
+ 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>;
+ };
+
+ gic: interrupt-controller@50801000 {
+ compatible = "arm,gic-400";
+ reg = <0x0 0x50801000 0 0x1000>, /* GICD */
+ <0x0 0x50802000 0 0x2000>, /* GICC */
+ <0x0 0x50804000 0 0x2000>, /* GICH */
+ <0x0 0x50806000 0 0x2000>; /* GICV */
+ #interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
+ interrupt-controller;
+ interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) |
+ IRQ_TYPE_LEVEL_HIGH)>;
+ };
+
+ uart0:serial@40700000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40700000 0x0 0x100>;
+ interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART0_GATE>;
+ status = "disabled";
+ };
+
+ uart1:serial@40710000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40710000 0x0 0x100>;
+ interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART1_GATE>;
+ status = "disabled";
+ };
+
+ uart2:serial@40720000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40720000 0x0 0x100>;
+ interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART2_GATE>;
+ status = "disabled";
+ };
+
+ uart3:serial@40730000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40730000 0x0 0x100>;
+ interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART3_GATE>;
+ status = "disabled";
+ };
+
+ uart4:serial@40740000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40740000 0x0 0x100>;
+ interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART4_GATE>;
+ status = "disabled";
+ };
+
+ uart5:serial@40750000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40750000 0x0 0x100>;
+ interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART5_GATE>;
+ status = "disabled";
+ };
+
+ uart6:serial@40760000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40760000 0x0 0x100>;
+ interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART6_GATE>;
+ status = "disabled";
+ };
+
+ uart7:serial@40770000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40770000 0x0 0x100>;
+ interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART7_GATE>;
+ status = "disabled";
+ };
+
+ uart8:serial@40780000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40780000 0x0 0x100>;
+ interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART8_GATE>;
+ status = "disabled";
+ };
+
+ uart9:serial@40790000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40790000 0x0 0x100>;
+ interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART9_GATE>;
+ status = "disabled";
+ };
+
+ uart10:serial@407a0000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x407a0000 0x0 0x100>;
+ interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART10_GATE>;
+ status = "disabled";
+ };
+
+ uart11:serial@407b0000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x407b0000 0x0 0x100>;
+ interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART11_GATE>;
+ status = "disabled";
+ };
+
+ uart12:serial@407c0000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x407c0000 0x0 0x100>;
+ interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART12_GATE>;
+ status = "disabled";
+ };
+
+ uart13:serial@407d0000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x407d0000 0x0 0x100>;
+ interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART13_GATE>;
+ status = "disabled";
+ };
+
+ uart14:serial@407e0000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x407e0000 0x0 0x100>;
+ interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART14_GATE>;
+ status = "disabled";
+ };
+
+ uart15:serial@407f0000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x407f0000 0x0 0x100>;
+ interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART15_GATE>;
+ status = "disabled";
+ };
+
+ uart16:serial@40880000 {
+ compatible = "nuvoton,ma35d1-uart";
+ reg = <0x0 0x40880000 0x0 0x100>;
+ interrupts = <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk UART16_GATE>;
+ status = "disabled";
+ };
+};