[PATCH v11 0/2] Add iop driver for Sunplus SP7021

[PATCH v11 0/2] Add iop driver for Sunplus SP7021

[Tony Huang]

Add iop driver for Sunplus SP7021 SOC

This is a patch series for iop driver for Sunplus SP7021 SOC.

Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD card and
etc.) into a single chip. It is designed for industrial control.

Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html

Tony Huang (2):
  dt-bindings: misc: Add iop yaml file for Sunplus SP7021
  misc: Add iop driver for Sunplus SP7021

 .../devicetree/bindings/misc/sunplus,iop.yaml      |  78 ++++
 MAINTAINERS                                        |   6 +
 drivers/misc/Kconfig                               |  23 ++
 drivers/misc/Makefile                              |   1 +
 drivers/misc/sunplus_iop.c                         | 411 +++++++++++++++++++++
 5 files changed, 519 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/sunplus,iop.yaml
 create mode 100644 drivers/misc/sunplus_iop.c

-- 
2.7.4

[PATCH v11 1/2] dt-bindings: misc: Add iop yaml file for Sunplus SP7021

[Tony Huang]

Add iop yaml file for Sunplus SP7021

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
---
Changes in v11:
 - Addressed comments from krzysztof.

 .../devicetree/bindings/misc/sunplus,iop.yaml      | 78 ++++++++++++++++++++++
 MAINTAINERS                                        |  5 ++
 2 files changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/sunplus,iop.yaml

diff --git a/Documentation/devicetree/bindings/misc/sunplus,iop.yaml b/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
new file mode 100644
index 0000000..ad1c4be
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) Sunplus Ltd. Co. 2021
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/sunplus-iop.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sunplus IOP(8051) controller
+
+maintainers:
+  - Tony Huang <tonyhuang.sunplus@gmail.com>
+
+description: |
+  Processor for I/O control, RTC wake-up procedure management,
+  and cooperation with CPU&PMC in power management.
+
+properties:
+  compatible:
+    enum:
+      - sunplus,sp7021-iop
+
+  reg:
+    items:
+      - description: IOP registers regions
+      - description: PMC registers regions
+      - description: MOON0 registers regions
+
+  reg-names:
+    items:
+      - const: iop
+      - const: iop_pmc
+      - const: moon0
+
+  interrupts:
+    items:
+      - description: IOP_INT0. IOP to system Interrupt 0.
+                     Sent by IOP to system RISC.
+      - description: IOP_INT1. IOP to System Interrupt 1.
+                     Sent by IOP to system RISC.
+
+  memory-region:
+    maxItems: 1
+
+  wakeup-gpios:
+    description: When the linux kernel system is powered off.
+      8051 is always powered. 8051 cand receive external signals
+      according to this gpio pin. 8051 receives external signal
+      through gpio pin. 8051 can power on linux kernel system.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - interrupts
+  - memory-region
+  - wakeup-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/gpio/gpio.h>
+    iop: iop@9c000400 {
+        compatible = "sunplus,sp7021-iop";
+        reg = <0x9c000400 0x80>, <0x9c003100 0x80>, <0x9c000000 0x80>;
+        reg-names = "iop", "iop_pmc", "moon0";
+        clocks = <&clkc 0x14>;
+        interrupt-parent = <&intc>;
+        interrupts = <41 IRQ_TYPE_LEVEL_HIGH>, <42 IRQ_TYPE_LEVEL_HIGH>;
+        memory-region = <&iop_reserve>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&iop_pins>;
+        wakeup-gpios = <&pctl 1 GPIO_ACTIVE_HIGH>;
+    };
\ No newline at end of file
diff --git a/MAINTAINERS b/MAINTAINERS
index fb18ce7..d64c8ed 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18242,6 +18242,11 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS IOP DRIVER
+M:	Tony Huang <tonyhuang.sunplus@gmail.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/misc/sunplus,iop.yaml
+
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
 M:	Rich Felker <dalias@libc.org>
-- 
2.7.4

Re: [PATCH v11 1/2] dt-bindings: misc: Add iop yaml file for Sunplus SP7021

[Krzysztof Kozlowski]

On 12/03/2022 17:16, Tony Huang wrote:
> Add iop yaml file for Sunplus SP7021
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> ---
> Changes in v11:
>  - Addressed comments from krzysztof.
> 
>  .../devicetree/bindings/misc/sunplus,iop.yaml      | 78 ++++++++++++++++++++++
>  MAINTAINERS                                        |  5 ++
>  2 files changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/sunplus,iop.yaml b/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> new file mode 100644
> index 0000000..ad1c4be
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) Sunplus Ltd. Co. 2021
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/sunplus-iop.yaml#

Still wrong path. This would be easily spotted if you test the bindings.
Please run dt_binding_check. It's a requirement.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sunplus IOP(8051) controller
> +
> +maintainers:
> +  - Tony Huang <tonyhuang.sunplus@gmail.com>
> +
> +description: |
> +  Processor for I/O control, RTC wake-up procedure management,
> +  and cooperation with CPU&PMC in power management.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sunplus,sp7021-iop
> +
> +  reg:
> +    items:
> +      - description: IOP registers regions
> +      - description: PMC registers regions
> +      - description: MOON0 registers regions
> +
> +  reg-names:
> +    items:
> +      - const: iop
> +      - const: iop_pmc
> +      - const: moon0
> +
> +  interrupts:
> +    items:
> +      - description: IOP_INT0. IOP to system Interrupt 0.
> +                     Sent by IOP to system RISC.
> +      - description: IOP_INT1. IOP to System Interrupt 1.
> +                     Sent by IOP to system RISC.
> +
> +  memory-region:
> +    maxItems: 1
> +
> +  wakeup-gpios:
> +    description: When the linux kernel system is powered off.
> +      8051 is always powered. 8051 cand receive external signals
> +      according to this gpio pin. 8051 receives external signal
> +      through gpio pin. 8051 can power on linux kernel system.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - interrupts
> +  - memory-region
> +  - wakeup-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    iop: iop@9c000400 {
> +        compatible = "sunplus,sp7021-iop";
> +        reg = <0x9c000400 0x80>, <0x9c003100 0x80>, <0x9c000000 0x80>;
> +        reg-names = "iop", "iop_pmc", "moon0";
> +        clocks = <&clkc 0x14>;
> +        interrupt-parent = <&intc>;
> +        interrupts = <41 IRQ_TYPE_LEVEL_HIGH>, <42 IRQ_TYPE_LEVEL_HIGH>;
> +        memory-region = <&iop_reserve>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&iop_pins>;
> +        wakeup-gpios = <&pctl 1 GPIO_ACTIVE_HIGH>;
> +    };
> \ No newline at end of file

You have a patch warning here.

Best regards,
Krzysztof

RE: [PATCH v11 1/2] dt-bindings: misc: Add iop yaml file for Sunplus SP7021

[Tony Huang 黃懷厚]

Dear Krzysztof:

> On 12/03/2022 17:16, Tony Huang wrote:
> > Add iop yaml file for Sunplus SP7021
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> > ---
> > Changes in v11:
> >  - Addressed comments from krzysztof.
> >
> >  .../devicetree/bindings/misc/sunplus,iop.yaml      | 78
> ++++++++++++++++++++++
> >  MAINTAINERS                                        |  5 ++
> >  2 files changed, 83 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> > b/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> > new file mode 100644
> > index 0000000..ad1c4be
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> > @@ -0,0 +1,78 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright
> > +(C) Sunplus Ltd. Co. 2021 %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/misc/sunplus-iop.yaml#
> 
> Still wrong path. This would be easily spotted if you test the bindings.
> Please run dt_binding_check. It's a requirement.

I have run dt_binding_check and don't see error.
Below is my modification
$id: http://devicetree.org/schemas/misc/sunplus,iop.yaml#

> 
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sunplus IOP(8051) controller
> > +
> > +maintainers:
> > +  - Tony Huang <tonyhuang.sunplus@gmail.com>
> > +
> > +description: |
> > +  Processor for I/O control, RTC wake-up procedure management,
> > +  and cooperation with CPU&PMC in power management.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sunplus,sp7021-iop
> > +
> > +  reg:
> > +    items:
> > +      - description: IOP registers regions
> > +      - description: PMC registers regions
> > +      - description: MOON0 registers regions
> > +
> > +  reg-names:
> > +    items:
> > +      - const: iop
> > +      - const: iop_pmc
> > +      - const: moon0
> > +
> > +  interrupts:
> > +    items:
> > +      - description: IOP_INT0. IOP to system Interrupt 0.
> > +                     Sent by IOP to system RISC.
> > +      - description: IOP_INT1. IOP to System Interrupt 1.
> > +                     Sent by IOP to system RISC.
> > +
> > +  memory-region:
> > +    maxItems: 1
> > +
> > +  wakeup-gpios:
> > +    description: When the linux kernel system is powered off.
> > +      8051 is always powered. 8051 cand receive external signals
> > +      according to this gpio pin. 8051 receives external signal
> > +      through gpio pin. 8051 can power on linux kernel system.
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - clocks
> > +  - interrupts
> > +  - memory-region
> > +  - wakeup-gpios
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    iop: iop@9c000400 {
> > +        compatible = "sunplus,sp7021-iop";
> > +        reg = <0x9c000400 0x80>, <0x9c003100 0x80>, <0x9c000000
> 0x80>;
> > +        reg-names = "iop", "iop_pmc", "moon0";
> > +        clocks = <&clkc 0x14>;
> > +        interrupt-parent = <&intc>;
> > +        interrupts = <41 IRQ_TYPE_LEVEL_HIGH>, <42
> IRQ_TYPE_LEVEL_HIGH>;
> > +        memory-region = <&iop_reserve>;
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&iop_pins>;
> > +        wakeup-gpios = <&pctl 1 GPIO_ACTIVE_HIGH>;
> > +    };
> > \ No newline at end of file

I will modify.

Re: [PATCH v11 1/2] dt-bindings: misc: Add iop yaml file for Sunplus SP7021

[Krzysztof Kozlowski]

On 12/03/2022 17:16, Tony Huang wrote:
> Add iop yaml file for Sunplus SP7021
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> ---
> Changes in v11:
>  - Addressed comments from krzysztof.
> 
>  .../devicetree/bindings/misc/sunplus,iop.yaml      | 78 ++++++++++++++++++++++
>  MAINTAINERS                                        |  5 ++
>  2 files changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/sunplus,iop.yaml b/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> new file mode 100644
> index 0000000..ad1c4be
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) Sunplus Ltd. Co. 2021
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/sunplus-iop.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sunplus IOP(8051) controller
> +
> +maintainers:
> +  - Tony Huang <tonyhuang.sunplus@gmail.com>
> +
> +description: |
> +  Processor for I/O control, RTC wake-up procedure management,
> +  and cooperation with CPU&PMC in power management.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sunplus,sp7021-iop
> +
> +  reg:
> +    items:
> +      - description: IOP registers regions
> +      - description: PMC registers regions
> +      - description: MOON0 registers regions
> +
> +  reg-names:
> +    items:
> +      - const: iop
> +      - const: iop_pmc
> +      - const: moon0
> +
> +  interrupts:
> +    items:
> +      - description: IOP_INT0. IOP to system Interrupt 0.
> +                     Sent by IOP to system RISC.
> +      - description: IOP_INT1. IOP to System Interrupt 1.
> +                     Sent by IOP to system RISC.
> +
> +  memory-region:
> +    maxItems: 1
> +
> +  wakeup-gpios:
> +    description: When the linux kernel system is powered off.
> +      8051 is always powered. 8051 cand receive external signals
> +      according to this gpio pin. 8051 receives external signal
> +      through gpio pin. 8051 can power on linux kernel system.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks

Why do you have clocks here but not in properties?

Best regards,
Krzysztof

RE: [PATCH v11 1/2] dt-bindings: misc: Add iop yaml file for Sunplus SP7021

[Tony Huang 黃懷厚]

Dear Krzysztof:

> On 12/03/2022 17:16, Tony Huang wrote:
> > Add iop yaml file for Sunplus SP7021
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> > ---
> > Changes in v11:
> >  - Addressed comments from krzysztof.
> >
> >  .../devicetree/bindings/misc/sunplus,iop.yaml      | 78
> ++++++++++++++++++++++
> >  MAINTAINERS                                        |  5 ++
> >  2 files changed, 83 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> > b/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> > new file mode 100644
> > index 0000000..ad1c4be
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> > @@ -0,0 +1,78 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright
> > +(C) Sunplus Ltd. Co. 2021 %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/misc/sunplus-iop.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sunplus IOP(8051) controller
> > +
> > +maintainers:
> > +  - Tony Huang <tonyhuang.sunplus@gmail.com>
> > +
> > +description: |
> > +  Processor for I/O control, RTC wake-up procedure management,
> > +  and cooperation with CPU&PMC in power management.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sunplus,sp7021-iop
> > +
> > +  reg:
> > +    items:
> > +      - description: IOP registers regions
> > +      - description: PMC registers regions
> > +      - description: MOON0 registers regions
> > +
> > +  reg-names:
> > +    items:
> > +      - const: iop
> > +      - const: iop_pmc
> > +      - const: moon0
> > +
> > +  interrupts:
> > +    items:
> > +      - description: IOP_INT0. IOP to system Interrupt 0.
> > +                     Sent by IOP to system RISC.
> > +      - description: IOP_INT1. IOP to System Interrupt 1.
> > +                     Sent by IOP to system RISC.
> > +
> > +  memory-region:
> > +    maxItems: 1
> > +
> > +  wakeup-gpios:
> > +    description: When the linux kernel system is powered off.
> > +      8051 is always powered. 8051 cand receive external signals
> > +      according to this gpio pin. 8051 receives external signal
> > +      through gpio pin. 8051 can power on linux kernel system.
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - clocks
> 
> Why do you have clocks here but not in properties?

I will modify. Add clocks in properties.

Re: [PATCH v11 1/2] dt-bindings: misc: Add iop yaml file for Sunplus SP7021

[Rob Herring]

On Sun, 13 Mar 2022 00:16:04 +0800, Tony Huang wrote:
> Add iop yaml file for Sunplus SP7021
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> ---
> Changes in v11:
>  - Addressed comments from krzysztof.
> 
>  .../devicetree/bindings/misc/sunplus,iop.yaml      | 78 ++++++++++++++++++++++
>  MAINTAINERS                                        |  5 ++
>  2 files changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/misc/sunplus,iop.yaml:78:7: [error] no new line character at the end of file (new-line-at-end-of-file)

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/misc/sunplus,iop.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/misc/sunplus,iop.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/sunplus,iop.example.dt.yaml: iop@9c000400: 'clocks' does not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/sunplus,iop.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1604673

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021

[Tony Huang]

This driver is load 8051 bin code.
The IOP core is DQ8051, so also named IOP8051.
Need Install DQ8051, The DQ8051 bin file generated by keil C.
We will provide users with 8051 normal and power off source code.
Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source-
files-for-IOP
Users can follow the operation steps to generate normal.bin and 
poweroff.bin.
Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338
/26.+IOP8051 26.5?How To Create 8051 bin file

PMC in power management purpose:
Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
When the power off command is executed.
The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
power-off of the system.

Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
---
Changes in v11:
 - Addressed comments from Arnd Bergmann.
 - Addressed comments from krzysztof.

 MAINTAINERS                |   1 +
 drivers/misc/Kconfig       |  23 +++
 drivers/misc/Makefile      |   1 +
 drivers/misc/sunplus_iop.c | 411 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 436 insertions(+)
 create mode 100644 drivers/misc/sunplus_iop.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d64c8ed..c282e95 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18246,6 +18246,7 @@ SUNPLUS IOP DRIVER
 M:	Tony Huang <tonyhuang.sunplus@gmail.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/misc/sunplus,iop.yaml
+F:	drivers/misc/sunplus_iop.c
 
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 0f5a49f..e5f32d8 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -470,6 +470,29 @@ config HISI_HIKEY_USB
 	  switching between the dual-role USB-C port and the USB-A host ports
 	  using only one USB controller.
 
+config SUNPLUS_IOP
+	tristate "Sunplus IOP support"
+	default ARCH_SUNPLUS
+	help
+	  This driver is load 8051 bin code.
+	  The IOP core is DQ8051, so also named IOP8051.
+	  Need Install DQ8051, The DQ8051 bin file generated by keil C.
+	  We will provide users with 8051 normal and power off source code.
+	  Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
+	  How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source-files-for-IOP
+	  Users can follow the operation steps to generate normal.bin and poweroff.bin.
+	  Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051
+	  26.5?How To Create 8051 bin file
+
+	  PMC in power management purpose:
+	  Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
+	  When the power off command is executed.
+	  The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
+	  power-off of the system.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called sunplus_iop.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index a086197..eafeab6 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_DW_XDATA_PCIE)	+= dw-xdata-pcie.o
 obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
 obj-$(CONFIG_OCXL)		+= ocxl/
 obj-$(CONFIG_BCM_VK)		+= bcm-vk/
+obj-$(CONFIG_SUNPLUS_IOP)	+= sunplus_iop.o
 obj-y				+= cardreader/
 obj-$(CONFIG_PVPANIC)   	+= pvpanic/
 obj-$(CONFIG_HABANA_AI)		+= habanalabs/
diff --git a/drivers/misc/sunplus_iop.c b/drivers/misc/sunplus_iop.c
new file mode 100644
index 0000000..5bdce5e
--- /dev/null
+++ b/drivers/misc/sunplus_iop.c
@@ -0,0 +1,411 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * The IOP driver for Sunplus SP7021
+ *
+ * Copyright (C) 2021 Sunplus Technology Inc.
+ *
+ * All Rights Reserved.
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/firmware.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+
+/* IOP register offset */
+#define IOP_CONTROL	0x00
+#define IOP_DATA0	0x20
+#define IOP_DATA1	0x24
+#define IOP_DATA2	0x28
+#define IOP_DATA3	0x2c
+#define IOP_DATA4	0x30
+#define IOP_DATA5	0x34
+#define IOP_DATA6	0x38
+#define IOP_DATA7	0x3c
+#define IOP_DATA8	0x40
+#define IOP_DATA9	0x44
+#define IOP_DATA10	0x48
+#define IOP_DATA11	0x4c
+#define IOP_BASE_ADR_L	0x50
+#define IOP_BASE_ADR_H	0x54
+
+/* PMC register offset */
+#define IOP_PMC_TIMER		0x00
+#define IOP_PMC_CTRL		0x04
+#define IOP_XTAL27M_PASSWORD_I	0x08
+#define IOP_XTAL27M_PASSWORD_II	0x0c
+#define IOP_XTAL32K_PASSWORD_I	0x10
+#define IOP_XTAL32K_PASSWORD_II	0x14
+#define IOP_CLK27M_PASSWORD_I	0x18
+#define IOP_CLK27M_PASSWORD_II	0x1c
+#define IOP_PMC_TIMER2		0x20
+
+/* Max size of poweroff.bin that can be received */
+#define POWEROFF_CODE_MAX_SIZE 0x4000
+
+/* 8051 informs linux kerenl. 8051 has been switched to poweroff.bin code. */
+#define IOP_READY	0x0004
+#define RISC_READY	0x0008
+
+/* System linux kernel tells 8051 which  gpio pin to wake-up through. */
+#define WAKEUP_PIN	0xFE02
+
+/*
+ * There are 3 power domains in SP7021, AO domain, IOP domain,
+ * Default domain. Default domain is linux kernel system.
+ * System linux kernel tells 8051 to execute power off.
+ */
+#define DEFAULT_DOMAIN_POWEROFF	0x5331 /* AO&IOP domain on, Default domain off */
+#define DEFAULT_AND_IOP_DOMAIN_POWEROFF	0x5333 /* AO domain on, IOP&Default domain off */
+
+struct sp_iop {
+	struct device *dev;
+	struct clk *iopclk;
+	struct reset_control *rstc;
+	void __iomem *iop_regs;
+	void __iomem *pmc_regs;
+	void __iomem *moon0_regs;
+	int irq;
+	int gpio_wakeup;
+	resource_size_t iop_mem_start;
+	resource_size_t iop_mem_size;
+	unsigned char bin_code_mode;
+};
+
+static struct sp_iop *iop_poweroff;
+
+static void sp_iop_load_normal_code(struct sp_iop *iop)
+{
+	const struct firmware *fw;
+	void __iomem *iop_kernel_base;
+	unsigned int reg, err;
+
+	err = request_firmware(&fw, "normal.bin", iop->dev);
+	if (err)
+		dev_err(iop->dev, "get bin file error\n");
+
+	iop_kernel_base = ioremap(iop->iop_mem_start, fw->size);
+	memset(iop_kernel_base, 0, fw->size);
+	memcpy(iop_kernel_base, fw->data, fw->size);
+	release_firmware(fw);
+
+	clk_prepare_enable(iop->iopclk);
+
+	reg = readl(iop->iop_regs + IOP_CONTROL);
+	reg |= 0x01;
+	writel(reg, iop->iop_regs + IOP_CONTROL);
+
+	reg = readl(iop->iop_regs + IOP_CONTROL);
+	reg &= ~(0x8000);
+	writel(reg, iop->iop_regs + IOP_CONTROL);
+
+	reg = readl(iop->iop_regs + IOP_CONTROL);
+	reg |= 0x0200;/* disable watchdog event reset IOP */
+	writel(reg, iop->iop_regs + IOP_CONTROL);
+
+	reg = (iop->iop_mem_start & 0xFFFF);
+	writel(reg, iop->iop_regs + IOP_BASE_ADR_L);
+	reg	= (iop->iop_mem_start >> 16);
+	writel(reg, iop->iop_regs + IOP_BASE_ADR_H);
+
+	reg = readl(iop->iop_regs + IOP_CONTROL);
+	reg &= ~(0x01);
+	writel(reg, iop->iop_regs + IOP_CONTROL);
+	iop->bin_code_mode = 0;
+}
+
+static void sp_iop_load_poweroff_code(struct sp_iop *iop)
+{
+	const struct firmware *fw;
+	void __iomem *iop_kernel_base;
+	unsigned int reg, err;
+
+	err = request_firmware(&fw, "poweroff.bin", iop->dev);
+	if (err)
+		dev_err(iop->dev, "get bin file error\n");
+
+	iop_kernel_base = ioremap(iop->iop_mem_start, POWEROFF_CODE_MAX_SIZE);
+	memset(iop_kernel_base, 0, POWEROFF_CODE_MAX_SIZE);
+	memcpy(iop_kernel_base, fw->data, POWEROFF_CODE_MAX_SIZE);
+	release_firmware(fw);
+
+	clk_prepare_enable(iop->iopclk);
+
+	reg = readl(iop->iop_regs + IOP_CONTROL);
+	reg |= 0x01;
+	writel(reg, iop->iop_regs + IOP_CONTROL);
+
+	reg = readl(iop->iop_regs + IOP_CONTROL);
+	reg &= ~(0x8000);
+	writel(reg, iop->iop_regs + IOP_CONTROL);
+
+	reg = readl(iop->iop_regs + IOP_CONTROL);
+	reg |= 0x0200;/* disable watchdog event reset IOP */
+	writel(reg, iop->iop_regs + IOP_CONTROL);
+
+	reg = (iop->iop_mem_start & 0xFFFF);
+	writel(reg, iop->iop_regs + IOP_BASE_ADR_L);
+	reg = (iop->iop_mem_start >> 16);
+	writel(reg, iop->iop_regs + IOP_BASE_ADR_H);
+
+	reg = readl(iop->iop_regs + IOP_CONTROL);
+	reg &= ~(0x01);
+	writel(reg, iop->iop_regs + IOP_CONTROL);
+	iop->bin_code_mode = 1;
+}
+
+static int sp_iop_default_iop_domain_poweroff(struct device *dev, struct sp_iop *iop)
+{
+	unsigned int reg;
+	int ret, value;
+
+	/* PMC set */
+	writel(0x00010001, iop->pmc_regs + IOP_PMC_TIMER);
+	reg = readl(iop->pmc_regs + IOP_PMC_CTRL);
+	/* disable system reset PMC, enalbe power down IOP Domain, enable gating clock 27Mhz */
+	reg |= 0x23;
+	writel(reg, iop->pmc_regs + IOP_PMC_CTRL);
+
+	writel(0x55aa00ff, iop->pmc_regs + IOP_XTAL27M_PASSWORD_I);
+	writel(0x00ff55aa, iop->pmc_regs + IOP_XTAL27M_PASSWORD_II);
+	writel(0xaa00ff55, iop->pmc_regs + IOP_XTAL32K_PASSWORD_I);
+	writel(0xff55aa00, iop->pmc_regs + IOP_XTAL32K_PASSWORD_II);
+	writel(0xaaff0055, iop->pmc_regs + IOP_CLK27M_PASSWORD_I);
+	writel(0x5500aaff, iop->pmc_regs + IOP_CLK27M_PASSWORD_II);
+	writel(0x01000100, iop->pmc_regs + IOP_PMC_TIMER2);
+
+	reg = readl(iop->moon0_regs + 32 * 4 * 1 + 4 * 2);
+	reg |= 0x08000800;
+	writel(reg, (iop->moon0_regs + 32 * 4 * 1 + 4 * 2));
+
+	writel(WAKEUP_PIN, iop->iop_regs + IOP_DATA0);
+	writel(iop->gpio_wakeup, iop->iop_regs + IOP_DATA1);
+
+	ret = readl_poll_timeout(iop->iop_regs + IOP_DATA2, value,
+				 (value & IOP_READY) == IOP_READY, 1000, 10000);
+	if (ret) {
+		dev_err(dev, "timed out\n");
+		return ret;
+	}
+
+	/*
+	 * IOP_DATA0~11(8051-RSIC mailbox register 0~11)
+	 * Mailbox is register that can be read and written by 8051 and System,
+	 * used to exchange information with system.
+	 * System fill in 0 to mailbox register 5.
+	 * System fill in 0x60 mailbox register 6.
+	 */
+	reg = RISC_READY;
+	writel(reg, iop->iop_regs + IOP_DATA2);
+	reg = 0x0000;
+	writel(reg, iop->iop_regs + IOP_DATA5);
+	reg = 0x0060;
+	writel(reg, iop->iop_regs + IOP_DATA6);
+	/*
+	 * After 8051 reads  mailbox register 5 = 0 and mailbox register 6 = 0x60,
+	 * and fill in 0xaaaa in mailbox register 7.
+	 * System reads mailbox register 7 = 0xaaaa, it can be donfirmed
+	 * that 8051 is executing poweroff code.
+	 */
+	ret = readl_poll_timeout(iop->iop_regs + IOP_DATA7, value,
+				 value == 0xaaaa, 1000, 10000);
+	if (ret) {
+		dev_err(dev, "timed out\n");
+		return ret;
+	}
+
+	/* 8051 bin file call Ultra low function. */
+	writel(0xdd, iop->iop_regs + IOP_DATA1);
+	/*
+	 * When the execution is here, the system linux kernel
+	 * is about to be powered off
+	 * The purpose of mdelay(10): Do not let the system linux
+	 * kernel continue to run other programs.
+	 */
+	mdelay(10);
+	return 0;
+}
+
+static int sp_iop_default_domain_poweroff(struct device *dev, struct sp_iop *iop)
+{
+	int ret, value;
+
+	ret = readl_poll_timeout(iop->iop_regs + IOP_DATA2, value,
+				 (value & IOP_READY) == IOP_READY, 1000, 10000);
+	if (ret) {
+		dev_err(dev, "timed out\n");
+		return ret;
+	}
+	/*
+	 * IOP_DATA0~11(8051-RSIC mailbox register 0~11)
+	 * Mailbox is register that can be read and written by 8051 and System,
+	 * used to exchange information with system.
+	 * System fill in 0 to mailbox register 5.
+	 * System fill in 0x60 mailbox register 6.
+	 */
+	writel(RISC_READY, iop->iop_regs + IOP_DATA2);
+	writel(0x0000, iop->iop_regs + IOP_DATA5);
+	writel(0x0060, iop->iop_regs + IOP_DATA6);
+	/*
+	 * After 8051 reads  mailbox register 5 = 0 and mailbox register 6 = 0x60,
+	 * and fill in 0xaaaa in mailbox register 7.
+	 * System reads mailbox register 7 = 0xaaaa, it can be donfirmed
+	 * that 8051 is executing poweroff code.
+	 */
+	ret = readl_poll_timeout(iop->iop_regs + IOP_DATA7, value,
+				 value == 0xaaaa, 1000, 10000);
+	if (ret) {
+		dev_err(dev, "timed out\n");
+		return ret;
+	}
+
+	/* 8051 bin file call poweroff function. */
+	writel(0xee, iop->iop_regs + IOP_DATA1);
+	/*
+	 * When the execution is here, the system linux kernel
+	 * is about to be powered off
+	 * The purpose of mdelay(10): Do not let the system linux
+	 * kernel continue to run other programs.
+	 */
+	mdelay(10);
+	return 0;
+}
+
+static void sp_iop_poweroff(void)
+{
+	struct sp_iop *iop = iop_poweroff;
+	unsigned int ret, value;
+
+	value = readl(iop->iop_regs + IOP_DATA11);
+	/*
+	 * When system linux kernel is power off, 8051 is to execute poweroff.bin code.
+	 * Standby.bin code cannot exceed 16K bytes. Because 8051 Icache size is 16k.
+	 * 8051 runs with poweroff.bin code in the Icache before the system(linux kernel)
+	 * is poweroff.
+	 */
+	sp_iop_load_poweroff_code(iop);
+
+	ret = readl_poll_timeout(iop->iop_regs + IOP_DATA0, value,
+				 value == 0x2222, 1000, 100000);
+	if (ret)
+		dev_warn(iop->dev, "timed out\n");
+
+	if (value == DEFAULT_DOMAIN_POWEROFF)
+		sp_iop_default_domain_poweroff(iop->dev, iop);
+	else
+		sp_iop_default_iop_domain_poweroff(iop->dev, iop);
+}
+
+static int sp_iop_get_resources(struct platform_device *pdev, struct sp_iop *p_sp_iop_info)
+{
+	struct resource *r;
+
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop");
+	p_sp_iop_info->iop_regs = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(p_sp_iop_info->iop_regs)) {
+		dev_err(&pdev->dev, "ioremap fail\n");
+		return PTR_ERR(p_sp_iop_info->iop_regs);
+	}
+
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop_pmc");
+	p_sp_iop_info->pmc_regs = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(p_sp_iop_info->pmc_regs)) {
+		dev_err(&pdev->dev, "ioremap fail\n");
+		return PTR_ERR(p_sp_iop_info->pmc_regs);
+	}
+
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon0");
+	p_sp_iop_info->moon0_regs = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(p_sp_iop_info->moon0_regs)) {
+		dev_err(&pdev->dev, "ioremap fail\n");
+		return PTR_ERR(p_sp_iop_info->moon0_regs);
+	}
+	return 0;
+}
+
+static int sp_iop_platform_driver_probe(struct platform_device *pdev)
+{
+	int ret = -ENXIO;
+	int rc;
+	struct sp_iop *iop;
+	struct device_node *memnp;
+	struct resource mem_res;
+
+	iop = devm_kzalloc(&pdev->dev, sizeof(struct sp_iop), GFP_KERNEL);
+	if (!iop) {
+		ret = -ENOMEM;
+		goto fail_kmalloc;
+	}
+
+	ret = sp_iop_get_resources(pdev, iop);
+
+	/* Get reserve address. */
+	memnp = of_parse_phandle(pdev->dev.of_node, "memory-region", 0);
+	if (!memnp) {
+		dev_err(&pdev->dev, "no memory-region node\n");
+		return -EINVAL;
+	}
+
+	rc = of_address_to_resource(memnp, 0, &mem_res);
+	of_node_put(memnp);
+	if (rc) {
+		dev_err(&pdev->dev, "failed to translate memory-region to a resource\n");
+		return -EINVAL;
+	}
+
+	iop->dev = &pdev->dev;
+	iop->iop_mem_start = mem_res.start;
+	iop->iop_mem_size = resource_size(&mem_res);
+	iop->iopclk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(iop->iopclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(iop->iopclk),
+					    "devm_clk_get fail\n");
+
+	/*
+	 * 8051 has two bin files, normal.bin and poweroff.bin in rootfs.
+	 * Normally, 8051 executes normal.bin code. Normal.bin code size can exceed 16K.
+	 */
+	sp_iop_load_normal_code(iop);
+
+	platform_set_drvdata(pdev, iop);
+	iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node, "iop-wakeup", 0);
+	iop_poweroff = iop;
+	pm_power_off = sp_iop_poweroff;
+
+	return 0;
+
+fail_kmalloc:
+	return ret;
+}
+
+static int sp_iop_remove(struct platform_device *pdev)
+{
+	pm_power_off = NULL;
+	return 0;
+}
+
+static const struct of_device_id sp_iop_of_match[] = {
+	{ .compatible = "sunplus,sp7021-iop" },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, sp_iop_of_match);
+
+static struct platform_driver sp_iop_platform_driver = {
+	.probe		= sp_iop_platform_driver_probe,
+	.remove		= sp_iop_remove,
+	.driver = {
+		.name	= "sunplus,sp7021-iop",
+		.of_match_table = sp_iop_of_match,
+	}
+};
+
+module_platform_driver(sp_iop_platform_driver);
+
+MODULE_AUTHOR("Tony Huang <tonyhuang.sunplus@gmail.com>");
+MODULE_DESCRIPTION("Sunplus IOP Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021

[Krzysztof Kozlowski]

On 12/03/2022 17:16, Tony Huang wrote:
> This driver is load 8051 bin code.
> The IOP core is DQ8051, so also named IOP8051.
> Need Install DQ8051, The DQ8051 bin file generated by keil C.
> We will provide users with 8051 normal and power off source code.
> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source-
> files-for-IOP
> Users can follow the operation steps to generate normal.bin and 
> poweroff.bin.
> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338
> /26.+IOP8051 26.5?How To Create 8051 bin file
> 
> PMC in power management purpose:
> Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> When the power off command is executed.
> The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
> power-off of the system.
> 
> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> ---
> Changes in v11:
>  - Addressed comments from Arnd Bergmann.

How did you address Arnd's comments about splitting the driver to proper
parts? drivers/clk and drivers/power/reset?

>  - Addressed comments from krzysztof.
> 
>  MAINTAINERS                |   1 +
>  drivers/misc/Kconfig       |  23 +++
>  drivers/misc/Makefile      |   1 +
>  drivers/misc/sunplus_iop.c | 411 +++++++++++++++++++++++++++++++++++++++++++++

The driver looks like SoC specific driver. Why did you put it in
drivers/misc/, not in usual place - drivers/soc/? sp_iop_poweroff is
still here.

>  4 files changed, 436 insertions(+)
>  create mode 100644 drivers/misc/sunplus_iop.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d64c8ed..c282e95 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18246,6 +18246,7 @@ SUNPLUS IOP DRIVER
>  M:	Tony Huang <tonyhuang.sunplus@gmail.com>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> +F:	drivers/misc/sunplus_iop.c
>  
>  SUPERH
>  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 0f5a49f..e5f32d8 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -470,6 +470,29 @@ config HISI_HIKEY_USB
>  	  switching between the dual-role USB-C port and the USB-A host ports
>  	  using only one USB controller.
>  
> +config SUNPLUS_IOP
> +	tristate "Sunplus IOP support"
> +	default ARCH_SUNPLUS
> +	help
> +	  This driver is load 8051 bin code.
> +	  The IOP core is DQ8051, so also named IOP8051.
> +	  Need Install DQ8051, The DQ8051 bin file generated by keil C.
> +	  We will provide users with 8051 normal and power off source code.
> +	  Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
> +	  How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source-files-for-IOP
> +	  Users can follow the operation steps to generate normal.bin and poweroff.bin.
> +	  Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051
> +	  26.5?How To Create 8051 bin file
> +
> +	  PMC in power management purpose:
> +	  Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> +	  When the power off command is executed.
> +	  The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
> +	  power-off of the system.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called sunplus_iop.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index a086197..eafeab6 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_DW_XDATA_PCIE)	+= dw-xdata-pcie.o
>  obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
>  obj-$(CONFIG_OCXL)		+= ocxl/
>  obj-$(CONFIG_BCM_VK)		+= bcm-vk/
> +obj-$(CONFIG_SUNPLUS_IOP)	+= sunplus_iop.o
>  obj-y				+= cardreader/
>  obj-$(CONFIG_PVPANIC)   	+= pvpanic/
>  obj-$(CONFIG_HABANA_AI)		+= habanalabs/
> diff --git a/drivers/misc/sunplus_iop.c b/drivers/misc/sunplus_iop.c
> new file mode 100644
> index 0000000..5bdce5e
> --- /dev/null
> +++ b/drivers/misc/sunplus_iop.c
> @@ -0,0 +1,411 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * The IOP driver for Sunplus SP7021
> + *
> + * Copyright (C) 2021 Sunplus Technology Inc.
> + *
> + * All Rights Reserved.
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/firmware.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +
> +/* IOP register offset */
> +#define IOP_CONTROL	0x00
> +#define IOP_DATA0	0x20
> +#define IOP_DATA1	0x24
> +#define IOP_DATA2	0x28
> +#define IOP_DATA3	0x2c
> +#define IOP_DATA4	0x30
> +#define IOP_DATA5	0x34
> +#define IOP_DATA6	0x38
> +#define IOP_DATA7	0x3c
> +#define IOP_DATA8	0x40
> +#define IOP_DATA9	0x44
> +#define IOP_DATA10	0x48
> +#define IOP_DATA11	0x4c
> +#define IOP_BASE_ADR_L	0x50
> +#define IOP_BASE_ADR_H	0x54
> +
> +/* PMC register offset */
> +#define IOP_PMC_TIMER		0x00
> +#define IOP_PMC_CTRL		0x04
> +#define IOP_XTAL27M_PASSWORD_I	0x08
> +#define IOP_XTAL27M_PASSWORD_II	0x0c
> +#define IOP_XTAL32K_PASSWORD_I	0x10
> +#define IOP_XTAL32K_PASSWORD_II	0x14
> +#define IOP_CLK27M_PASSWORD_I	0x18
> +#define IOP_CLK27M_PASSWORD_II	0x1c
> +#define IOP_PMC_TIMER2		0x20
> +
> +/* Max size of poweroff.bin that can be received */
> +#define POWEROFF_CODE_MAX_SIZE 0x4000
> +
> +/* 8051 informs linux kerenl. 8051 has been switched to poweroff.bin code. */
> +#define IOP_READY	0x0004
> +#define RISC_READY	0x0008
> +
> +/* System linux kernel tells 8051 which  gpio pin to wake-up through. */
> +#define WAKEUP_PIN	0xFE02
> +
> +/*
> + * There are 3 power domains in SP7021, AO domain, IOP domain,
> + * Default domain. Default domain is linux kernel system.
> + * System linux kernel tells 8051 to execute power off.
> + */
> +#define DEFAULT_DOMAIN_POWEROFF	0x5331 /* AO&IOP domain on, Default domain off */
> +#define DEFAULT_AND_IOP_DOMAIN_POWEROFF	0x5333 /* AO domain on, IOP&Default domain off */
> +
> +struct sp_iop {
> +	struct device *dev;
> +	struct clk *iopclk;
> +	struct reset_control *rstc;
> +	void __iomem *iop_regs;
> +	void __iomem *pmc_regs;
> +	void __iomem *moon0_regs;
> +	int irq;
> +	int gpio_wakeup;
> +	resource_size_t iop_mem_start;
> +	resource_size_t iop_mem_size;
> +	unsigned char bin_code_mode;
> +};
> +
> +static struct sp_iop *iop_poweroff;
> +
> +static void sp_iop_load_normal_code(struct sp_iop *iop)
> +{
> +	const struct firmware *fw;
> +	void __iomem *iop_kernel_base;
> +	unsigned int reg, err;
> +
> +	err = request_firmware(&fw, "normal.bin", iop->dev);
> +	if (err)
> +		dev_err(iop->dev, "get bin file error\n");
> +
> +	iop_kernel_base = ioremap(iop->iop_mem_start, fw->size);
> +	memset(iop_kernel_base, 0, fw->size);
> +	memcpy(iop_kernel_base, fw->data, fw->size);
> +	release_firmware(fw);
> +
> +	clk_prepare_enable(iop->iopclk);

where do you disable the clock?

> +
> +	reg = readl(iop->iop_regs + IOP_CONTROL);
> +	reg |= 0x01;
> +	writel(reg, iop->iop_regs + IOP_CONTROL);
> +

(...)

> +static int sp_iop_get_resources(struct platform_device *pdev, struct sp_iop *p_sp_iop_info)
> +{
> +	struct resource *r;
> +
> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop");
> +	p_sp_iop_info->iop_regs = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(p_sp_iop_info->iop_regs)) {
> +		dev_err(&pdev->dev, "ioremap fail\n");
> +		return PTR_ERR(p_sp_iop_info->iop_regs);
> +	}
> +
> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop_pmc");
> +	p_sp_iop_info->pmc_regs = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(p_sp_iop_info->pmc_regs)) {
> +		dev_err(&pdev->dev, "ioremap fail\n");
> +		return PTR_ERR(p_sp_iop_info->pmc_regs);
> +	}
> +
> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon0");
> +	p_sp_iop_info->moon0_regs = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(p_sp_iop_info->moon0_regs)) {
> +		dev_err(&pdev->dev, "ioremap fail\n");
> +		return PTR_ERR(p_sp_iop_info->moon0_regs);
> +	}
> +	return 0;

https://lore.kernel.org/all/16d98dfa-66f9-f9a4-08c9-d68d78c33b23@canonical.com/
You received a comment about adding blank lines before return. This has
to be done everywhere.

> +}
> +
> +static int sp_iop_platform_driver_probe(struct platform_device *pdev)

Do not add "platform_driver" suffix to functions. "_probe" is enough.

> +{
> +	int ret = -ENXIO;
> +	int rc;
> +	struct sp_iop *iop;
> +	struct device_node *memnp;
> +	struct resource mem_res;
> +
> +	iop = devm_kzalloc(&pdev->dev, sizeof(struct sp_iop), GFP_KERNEL);
> +	if (!iop) {
> +		ret = -ENOMEM;
> +		goto fail_kmalloc;
> +	}
> +
> +	ret = sp_iop_get_resources(pdev, iop);
> +
> +	/* Get reserve address. */
> +	memnp = of_parse_phandle(pdev->dev.of_node, "memory-region", 0);
> +	if (!memnp) {
> +		dev_err(&pdev->dev, "no memory-region node\n");
> +		return -EINVAL;
> +	}
> +
> +	rc = of_address_to_resource(memnp, 0, &mem_res);
> +	of_node_put(memnp);
> +	if (rc) {
> +		dev_err(&pdev->dev, "failed to translate memory-region to a resource\n");
> +		return -EINVAL;
> +	}
> +
> +	iop->dev = &pdev->dev;
> +	iop->iop_mem_start = mem_res.start;
> +	iop->iop_mem_size = resource_size(&mem_res);
> +	iop->iopclk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(iop->iopclk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(iop->iopclk),
> +					    "devm_clk_get fail\n");

Clocks are not documented but required in bindings?

> +
> +	/*
> +	 * 8051 has two bin files, normal.bin and poweroff.bin in rootfs.
> +	 * Normally, 8051 executes normal.bin code. Normal.bin code size can exceed 16K.
> +	 */
> +	sp_iop_load_normal_code(iop);
> +
> +	platform_set_drvdata(pdev, iop);
> +	iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node, "iop-wakeup", 0);

It's not in the bindings. Do not add undocumented properties. Everything
has to be in the bindings. I don't see usage of generic wakeup-gpios, so
this all looks untested. You didn't run this code, did you?

> +	iop_poweroff = iop;
> +	pm_power_off = sp_iop_poweroff;
> +
> +	return 0;
> +
> +fail_kmalloc:
> +	return ret;
> +}
> +
> +static int sp_iop_remove(struct platform_device *pdev)
> +{
> +	pm_power_off = NULL;
> +	return 0;
> +}
> +
> +static const struct of_device_id sp_iop_of_match[] = {
> +	{ .compatible = "sunplus,sp7021-iop" },
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, sp_iop_of_match);
> +
> +static struct platform_driver sp_iop_platform_driver = {
> +	.probe		= sp_iop_platform_driver_probe,
> +	.remove		= sp_iop_remove,
> +	.driver = {
> +		.name	= "sunplus,sp7021-iop",
> +		.of_match_table = sp_iop_of_match,
> +	}
> +};
> +
> +module_platform_driver(sp_iop_platform_driver);
> +
> +MODULE_AUTHOR("Tony Huang <tonyhuang.sunplus@gmail.com>");
> +MODULE_DESCRIPTION("Sunplus IOP Driver");
> +MODULE_LICENSE("GPL v2");


Best regards,
Krzysztof

RE: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021

[Tony Huang 黃懷厚]

Dear Krzysztof:

> On 12/03/2022 17:16, Tony Huang wrote:
> > This driver is load 8051 bin code.
> > The IOP core is DQ8051, so also named IOP8051.
> > Need Install DQ8051, The DQ8051 bin file generated by keil C.
> > We will provide users with 8051 normal and power off source code.
> > Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
> >
> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source-
> > files-for-IOP
> > Users can follow the operation steps to generate normal.bin and
> > poweroff.bin.
> > Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338
> > /26.+IOP8051 26.5?How To Create 8051 bin file
> >
> > PMC in power management purpose:
> > Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> > When the power off command is executed.
> > The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
> > power-off of the system.
> >
> > Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> > ---
> > Changes in v11:
> >  - Addressed comments from Arnd Bergmann.
> 
> How did you address Arnd's comments about splitting the driver to proper
> parts? drivers/clk and drivers/power/reset?
> 

drivers/clk : SP7021 system has clock device driver (clk-sp7021.c).		
So I set the IOP clock through the following function.		
clk_prepare_enable(iop->iopclk);		
clk_disable_unprepare(iop->iopclk);		
		
drivers/power/reset : SP7021 system does not have a power off device driver.		

> >  - Addressed comments from krzysztof.
> >
> >  MAINTAINERS                |   1 +
> >  drivers/misc/Kconfig       |  23 +++
> >  drivers/misc/Makefile      |   1 +
> >  drivers/misc/sunplus_iop.c | 411
> > +++++++++++++++++++++++++++++++++++++++++++++
> 
> The driver looks like SoC specific driver. Why did you put it in drivers/misc/,
> not in usual place - drivers/soc/?

8051 is designed for processing I/O events, like receiving IR signal from remote controller, 		
taking care of power on or off requests from RTC, or other hardware events of external peripherals 		
even when power of main system is off.
So I put it in drivers/misc.

> sp_iop_poweroff is still here.

sp_iop_poweroff(): SP7021 system does not have a power off device driver.
				I have to put it here.

> >  4 files changed, 436 insertions(+)
> >  create mode 100644 drivers/misc/sunplus_iop.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index d64c8ed..c282e95 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18246,6 +18246,7 @@ SUNPLUS IOP DRIVER
> >  M:	Tony Huang <tonyhuang.sunplus@gmail.com>
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> > +F:	drivers/misc/sunplus_iop.c
> >
> >  SUPERH
> >  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index
> > 0f5a49f..e5f32d8 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -470,6 +470,29 @@ config HISI_HIKEY_USB
> >  	  switching between the dual-role USB-C port and the USB-A host ports
> >  	  using only one USB controller.
> >
> > +config SUNPLUS_IOP
> > +	tristate "Sunplus IOP support"
> > +	default ARCH_SUNPLUS
> > +	help
> > +	  This driver is load 8051 bin code.
> > +	  The IOP core is DQ8051, so also named IOP8051.
> > +	  Need Install DQ8051, The DQ8051 bin file generated by keil C.
> > +	  We will provide users with 8051 normal and power off source code.
> > +	  Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
> > +
> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source-
> files-for-IOP
> > +	  Users can follow the operation steps to generate normal.bin and
> poweroff.bin.
> > +	  Path:
> https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051
> > +	  26.5?How To Create 8051 bin file
> > +
> > +	  PMC in power management purpose:
> > +	  Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> > +	  When the power off command is executed.
> > +	  The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on
> and
> > +	  power-off of the system.
> > +
> > +	  This driver can also be built as a module.  If so, the module
> > +	  will be called sunplus_iop.
> > +
> >  source "drivers/misc/c2port/Kconfig"
> >  source "drivers/misc/eeprom/Kconfig"
> >  source "drivers/misc/cb710/Kconfig"
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index
> > a086197..eafeab6 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -52,6 +52,7 @@ obj-$(CONFIG_DW_XDATA_PCIE)	+= dw-xdata-pcie.o
> >  obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
> >  obj-$(CONFIG_OCXL)		+= ocxl/
> >  obj-$(CONFIG_BCM_VK)		+= bcm-vk/
> > +obj-$(CONFIG_SUNPLUS_IOP)	+= sunplus_iop.o
> >  obj-y				+= cardreader/
> >  obj-$(CONFIG_PVPANIC)   	+= pvpanic/
> >  obj-$(CONFIG_HABANA_AI)		+= habanalabs/
> > diff --git a/drivers/misc/sunplus_iop.c b/drivers/misc/sunplus_iop.c
> > new file mode 100644 index 0000000..5bdce5e
> > --- /dev/null
> > +++ b/drivers/misc/sunplus_iop.c
> > @@ -0,0 +1,411 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * The IOP driver for Sunplus SP7021
> > + *
> > + * Copyright (C) 2021 Sunplus Technology Inc.
> > + *
> > + * All Rights Reserved.
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/firmware.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_gpio.h>
> > +
> > +/* IOP register offset */
> > +#define IOP_CONTROL	0x00
> > +#define IOP_DATA0	0x20
> > +#define IOP_DATA1	0x24
> > +#define IOP_DATA2	0x28
> > +#define IOP_DATA3	0x2c
> > +#define IOP_DATA4	0x30
> > +#define IOP_DATA5	0x34
> > +#define IOP_DATA6	0x38
> > +#define IOP_DATA7	0x3c
> > +#define IOP_DATA8	0x40
> > +#define IOP_DATA9	0x44
> > +#define IOP_DATA10	0x48
> > +#define IOP_DATA11	0x4c
> > +#define IOP_BASE_ADR_L	0x50
> > +#define IOP_BASE_ADR_H	0x54
> > +
> > +/* PMC register offset */
> > +#define IOP_PMC_TIMER		0x00
> > +#define IOP_PMC_CTRL		0x04
> > +#define IOP_XTAL27M_PASSWORD_I	0x08
> > +#define IOP_XTAL27M_PASSWORD_II	0x0c
> > +#define IOP_XTAL32K_PASSWORD_I	0x10
> > +#define IOP_XTAL32K_PASSWORD_II	0x14
> > +#define IOP_CLK27M_PASSWORD_I	0x18
> > +#define IOP_CLK27M_PASSWORD_II	0x1c
> > +#define IOP_PMC_TIMER2		0x20
> > +
> > +/* Max size of poweroff.bin that can be received */ #define
> > +POWEROFF_CODE_MAX_SIZE 0x4000
> > +
> > +/* 8051 informs linux kerenl. 8051 has been switched to poweroff.bin code.
> */
> > +#define IOP_READY	0x0004
> > +#define RISC_READY	0x0008
> > +
> > +/* System linux kernel tells 8051 which  gpio pin to wake-up through. */
> > +#define WAKEUP_PIN	0xFE02
> > +
> > +/*
> > + * There are 3 power domains in SP7021, AO domain, IOP domain,
> > + * Default domain. Default domain is linux kernel system.
> > + * System linux kernel tells 8051 to execute power off.
> > + */
> > +#define DEFAULT_DOMAIN_POWEROFF	0x5331 /* AO&IOP domain on,
> Default domain off */
> > +#define DEFAULT_AND_IOP_DOMAIN_POWEROFF	0x5333 /* AO domain
> on, IOP&Default domain off */
> > +
> > +struct sp_iop {
> > +	struct device *dev;
> > +	struct clk *iopclk;
> > +	struct reset_control *rstc;
> > +	void __iomem *iop_regs;
> > +	void __iomem *pmc_regs;
> > +	void __iomem *moon0_regs;
> > +	int irq;
> > +	int gpio_wakeup;
> > +	resource_size_t iop_mem_start;
> > +	resource_size_t iop_mem_size;
> > +	unsigned char bin_code_mode;
> > +};
> > +
> > +static struct sp_iop *iop_poweroff;
> > +
> > +static void sp_iop_load_normal_code(struct sp_iop *iop) {
> > +	const struct firmware *fw;
> > +	void __iomem *iop_kernel_base;
> > +	unsigned int reg, err;
> > +
> > +	err = request_firmware(&fw, "normal.bin", iop->dev);
> > +	if (err)
> > +		dev_err(iop->dev, "get bin file error\n");
> > +
> > +	iop_kernel_base = ioremap(iop->iop_mem_start, fw->size);
> > +	memset(iop_kernel_base, 0, fw->size);
> > +	memcpy(iop_kernel_base, fw->data, fw->size);
> > +	release_firmware(fw);
> > +
> > +	clk_prepare_enable(iop->iopclk);
> 
> where do you disable the clock?

I will add disable clock in sp_iop_remove().				
Below is my modification				
static int sp_iop_remove(struct platform_device *pdev)				
{				
	struct sp_iop *iop = iop_poweroff;			
	pm_power_off = NULL;			
	clk_disable_unprepare(iop->iopclk);			
				
	return 0;			
}				

> > +
> > +	reg = readl(iop->iop_regs + IOP_CONTROL);
> > +	reg |= 0x01;
> > +	writel(reg, iop->iop_regs + IOP_CONTROL);
> > +
> 
> (...)
> 
> > +static int sp_iop_get_resources(struct platform_device *pdev, struct
> > +sp_iop *p_sp_iop_info) {
> > +	struct resource *r;
> > +
> > +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop");
> > +	p_sp_iop_info->iop_regs = devm_ioremap_resource(&pdev->dev, r);
> > +	if (IS_ERR(p_sp_iop_info->iop_regs)) {
> > +		dev_err(&pdev->dev, "ioremap fail\n");
> > +		return PTR_ERR(p_sp_iop_info->iop_regs);
> > +	}
> > +
> > +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "iop_pmc");
> > +	p_sp_iop_info->pmc_regs = devm_ioremap_resource(&pdev->dev, r);
> > +	if (IS_ERR(p_sp_iop_info->pmc_regs)) {
> > +		dev_err(&pdev->dev, "ioremap fail\n");
> > +		return PTR_ERR(p_sp_iop_info->pmc_regs);
> > +	}
> > +
> > +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon0");
> > +	p_sp_iop_info->moon0_regs = devm_ioremap_resource(&pdev->dev, r);
> > +	if (IS_ERR(p_sp_iop_info->moon0_regs)) {
> > +		dev_err(&pdev->dev, "ioremap fail\n");
> > +		return PTR_ERR(p_sp_iop_info->moon0_regs);
> > +	}
> > +	return 0;
> 
> https://lore.kernel.org/all/16d98dfa-66f9-f9a4-08c9-d68d78c33b23@canonical
> .com/
> You received a comment about adding blank lines before return. This has to be
> done everywhere.

OK, I will modify it.		Added blank line before return everywhere.	

> > +}
> > +
> > +static int sp_iop_platform_driver_probe(struct platform_device *pdev)
> 
> Do not add "platform_driver" suffix to functions. "_probe" is enough.
> 

Below is my modification:				
static int sp_iop_probe(struct platform_device *pdev)				

> > +{
> > +	int ret = -ENXIO;
> > +	int rc;
> > +	struct sp_iop *iop;
> > +	struct device_node *memnp;
> > +	struct resource mem_res;
> > +
> > +	iop = devm_kzalloc(&pdev->dev, sizeof(struct sp_iop), GFP_KERNEL);
> > +	if (!iop) {
> > +		ret = -ENOMEM;
> > +		goto fail_kmalloc;
> > +	}
> > +
> > +	ret = sp_iop_get_resources(pdev, iop);
> > +
> > +	/* Get reserve address. */
> > +	memnp = of_parse_phandle(pdev->dev.of_node, "memory-region", 0);
> > +	if (!memnp) {
> > +		dev_err(&pdev->dev, "no memory-region node\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	rc = of_address_to_resource(memnp, 0, &mem_res);
> > +	of_node_put(memnp);
> > +	if (rc) {
> > +		dev_err(&pdev->dev, "failed to translate memory-region to a
> resource\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	iop->dev = &pdev->dev;
> > +	iop->iop_mem_start = mem_res.start;
> > +	iop->iop_mem_size = resource_size(&mem_res);
> > +	iop->iopclk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(iop->iopclk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(iop->iopclk),
> > +					    "devm_clk_get fail\n");
> 
> Clocks are not documented but required in bindings?
> 

I will added clocks in properties.		

> > +
> > +	/*
> > +	 * 8051 has two bin files, normal.bin and poweroff.bin in rootfs.
> > +	 * Normally, 8051 executes normal.bin code. Normal.bin code size can
> exceed 16K.
> > +	 */
> > +	sp_iop_load_normal_code(iop);
> > +
> > +	platform_set_drvdata(pdev, iop);
> > +	iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node,
> > +"iop-wakeup", 0);
> 
> It's not in the bindings. Do not add undocumented properties. Everything has to
> be in the bindings. I don't see usage of generic wakeup-gpios, so this all looks
> untested. You didn't run this code, did you?
> 

I wrote the wrong name.			
Below is my modification:			
iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node, "wakeup-gpios", 0);			

Re: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021

[Krzysztof Kozlowski]

On 14/03/2022 07:08, Tony Huang 黃懷厚 wrote:
> Dear Krzysztof:
> 
>> On 12/03/2022 17:16, Tony Huang wrote:
>>> This driver is load 8051 bin code.
>>> The IOP core is DQ8051, so also named IOP8051.
>>> Need Install DQ8051, The DQ8051 bin file generated by keil C.
>>> We will provide users with 8051 normal and power off source code.
>>> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
>>>
>> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source-
>>> files-for-IOP
>>> Users can follow the operation steps to generate normal.bin and
>>> poweroff.bin.
>>> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338
>>> /26.+IOP8051 26.5?How To Create 8051 bin file
>>>
>>> PMC in power management purpose:
>>> Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
>>> When the power off command is executed.
>>> The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
>>> power-off of the system.
>>>
>>> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
>>> ---
>>> Changes in v11:
>>>  - Addressed comments from Arnd Bergmann.
>>
>> How did you address Arnd's comments about splitting the driver to proper
>> parts? drivers/clk and drivers/power/reset?
>>
> 
> drivers/clk : SP7021 system has clock device driver (clk-sp7021.c).		
> So I set the IOP clock through the following function.		
> clk_prepare_enable(iop->iopclk);		
> clk_disable_unprepare(iop->iopclk);		
> 		
> drivers/power/reset : SP7021 system does not have a power off device driver.

What does it mean? The feedback was to split clk and reset features to
separate drivers and I still see only two patches here with a misc
driver. So how is his comments addressed? You did not reply in that thread.

> 
>>>  - Addressed comments from krzysztof.
>>>
>>>  MAINTAINERS                |   1 +
>>>  drivers/misc/Kconfig       |  23 +++
>>>  drivers/misc/Makefile      |   1 +
>>>  drivers/misc/sunplus_iop.c | 411
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>
>> The driver looks like SoC specific driver. Why did you put it in drivers/misc/,
>> not in usual place - drivers/soc/?
> 
> 8051 is designed for processing I/O events, like receiving IR signal from remote controller, 		
> taking care of power on or off requests from RTC, or other hardware events of external peripherals 		
> even when power of main system is off.
> So I put it in drivers/misc.

Is IOP8061 a separate device? Your datasheet is saying its embedded in
SP7021 SoC, so it is a soc driver. This does not fit misc driver
description (a "strange device") but a SoC driver description.

> 
>> sp_iop_poweroff is still here.
> 
> sp_iop_poweroff(): SP7021 system does not have a power off device driver.
> 				I have to put it here.

This should be rather in a reset driver, not in a misc one. What is your
plan for this driver? You described the hardware and judging by it,
there will be quite a lot of separate features so it's reasonable to
split the driver into separate logical elements. However keeping all in
the same place would be ok, if you do not plan to add any more features.
This would mean the driver will handle *only* reset and FW loading.

> 
>>>  4 files changed, 436 insertions(+)
>>>  create mode 100644 drivers/misc/sunplus_iop.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS index d64c8ed..c282e95 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -18246,6 +18246,7 @@ SUNPLUS IOP DRIVER
>>>  M:	Tony Huang <tonyhuang.sunplus@gmail.com>
>>>  S:	Maintained
>>>  F:	Documentation/devicetree/bindings/misc/sunplus,iop.yaml
>>> +F:	drivers/misc/sunplus_iop.c
>>>
>>>  SUPERH
>>>  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
>>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index
>>> 0f5a49f..e5f32d8 100644
>>> --- a/drivers/misc/Kconfig
>>> +++ b/drivers/misc/Kconfig
>>> @@ -470,6 +470,29 @@ config HISI_HIKEY_USB
>>>  	  switching between the dual-role USB-C port and the USB-A host ports
>>>  	  using only one USB controller.
>>>
>>> +config SUNPLUS_IOP
>>> +	tristate "Sunplus IOP support"
>>> +	default ARCH_SUNPLUS
>>> +	help
>>> +	  This driver is load 8051 bin code.
>>> +	  The IOP core is DQ8051, so also named IOP8051.
>>> +	  Need Install DQ8051, The DQ8051 bin file generated by keil C.
>>> +	  We will provide users with 8051 normal and power off source code.
>>> +	  Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
>>> +
>> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source-
>> files-for-IOP
>>> +	  Users can follow the operation steps to generate normal.bin and
>> poweroff.bin.
>>> +	  Path:
>> https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051
>>> +	  26.5?How To Create 8051 bin file
>>> +
>>> +	  PMC in power management purpose:
>>> +	  Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
>>> +	  When the power off command is executed.
>>> +	  The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on
>> and
>>> +	  power-off of the system.
>>> +
>>> +	  This driver can also be built as a module.  If so, the module
>>> +	  will be called sunplus_iop.
>>> +
>>>  source "drivers/misc/c2port/Kconfig"
>>>  source "drivers/misc/eeprom/Kconfig"
>>>  source "drivers/misc/cb710/Kconfig"
>>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index
>>> a086197..eafeab6 100644
>>> --- a/drivers/misc/Makefile
>>> +++ b/drivers/misc/Makefile
>>> @@ -52,6 +52,7 @@ obj-$(CONFIG_DW_XDATA_PCIE)	+= dw-xdata-pcie.o
>>>  obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
>>>  obj-$(CONFIG_OCXL)		+= ocxl/
>>>  obj-$(CONFIG_BCM_VK)		+= bcm-vk/
>>> +obj-$(CONFIG_SUNPLUS_IOP)	+= sunplus_iop.o
>>>  obj-y				+= cardreader/
>>>  obj-$(CONFIG_PVPANIC)   	+= pvpanic/
>>>  obj-$(CONFIG_HABANA_AI)		+= habanalabs/
>>> diff --git a/drivers/misc/sunplus_iop.c b/drivers/misc/sunplus_iop.c
>>> new file mode 100644 index 0000000..5bdce5e
>>> --- /dev/null
>>> +++ b/drivers/misc/sunplus_iop.c
>>> @@ -0,0 +1,411 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * The IOP driver for Sunplus SP7021
>>> + *
>>> + * Copyright (C) 2021 Sunplus Technology Inc.
>>> + *
>>> + * All Rights Reserved.
>>> + */
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/firmware.h>
>>> +#include <linux/iopoll.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_gpio.h>
>>> +
>>> +/* IOP register offset */
>>> +#define IOP_CONTROL	0x00
>>> +#define IOP_DATA0	0x20
>>> +#define IOP_DATA1	0x24
>>> +#define IOP_DATA2	0x28
>>> +#define IOP_DATA3	0x2c
>>> +#define IOP_DATA4	0x30
>>> +#define IOP_DATA5	0x34
>>> +#define IOP_DATA6	0x38
>>> +#define IOP_DATA7	0x3c
>>> +#define IOP_DATA8	0x40
>>> +#define IOP_DATA9	0x44
>>> +#define IOP_DATA10	0x48
>>> +#define IOP_DATA11	0x4c
>>> +#define IOP_BASE_ADR_L	0x50
>>> +#define IOP_BASE_ADR_H	0x54
>>> +
>>> +/* PMC register offset */
>>> +#define IOP_PMC_TIMER		0x00
>>> +#define IOP_PMC_CTRL		0x04
>>> +#define IOP_XTAL27M_PASSWORD_I	0x08
>>> +#define IOP_XTAL27M_PASSWORD_II	0x0c
>>> +#define IOP_XTAL32K_PASSWORD_I	0x10
>>> +#define IOP_XTAL32K_PASSWORD_II	0x14
>>> +#define IOP_CLK27M_PASSWORD_I	0x18
>>> +#define IOP_CLK27M_PASSWORD_II	0x1c
>>> +#define IOP_PMC_TIMER2		0x20
>>> +
>>> +/* Max size of poweroff.bin that can be received */ #define
>>> +POWEROFF_CODE_MAX_SIZE 0x4000
>>> +
>>> +/* 8051 informs linux kerenl. 8051 has been switched to poweroff.bin code.
>> */
>>> +#define IOP_READY	0x0004
>>> +#define RISC_READY	0x0008
>>> +
>>> +/* System linux kernel tells 8051 which  gpio pin to wake-up through. */
>>> +#define WAKEUP_PIN	0xFE02
>>> +
>>> +/*
>>> + * There are 3 power domains in SP7021, AO domain, IOP domain,
>>> + * Default domain. Default domain is linux kernel system.
>>> + * System linux kernel tells 8051 to execute power off.
>>> + */
>>> +#define DEFAULT_DOMAIN_POWEROFF	0x5331 /* AO&IOP domain on,
>> Default domain off */
>>> +#define DEFAULT_AND_IOP_DOMAIN_POWEROFF	0x5333 /* AO domain
>> on, IOP&Default domain off */
>>> +
>>> +struct sp_iop {
>>> +	struct device *dev;
>>> +	struct clk *iopclk;
>>> +	struct reset_control *rstc;
>>> +	void __iomem *iop_regs;
>>> +	void __iomem *pmc_regs;
>>> +	void __iomem *moon0_regs;
>>> +	int irq;
>>> +	int gpio_wakeup;
>>> +	resource_size_t iop_mem_start;
>>> +	resource_size_t iop_mem_size;
>>> +	unsigned char bin_code_mode;
>>> +};
>>> +
>>> +static struct sp_iop *iop_poweroff;
>>> +
>>> +static void sp_iop_load_normal_code(struct sp_iop *iop) {
>>> +	const struct firmware *fw;
>>> +	void __iomem *iop_kernel_base;
>>> +	unsigned int reg, err;
>>> +
>>> +	err = request_firmware(&fw, "normal.bin", iop->dev);
>>> +	if (err)
>>> +		dev_err(iop->dev, "get bin file error\n");
>>> +
>>> +	iop_kernel_base = ioremap(iop->iop_mem_start, fw->size);
>>> +	memset(iop_kernel_base, 0, fw->size);
>>> +	memcpy(iop_kernel_base, fw->data, fw->size);
>>> +	release_firmware(fw);
>>> +
>>> +	clk_prepare_enable(iop->iopclk);
>>
>> where do you disable the clock?
> 
> I will add disable clock in sp_iop_remove().				
> Below is my modification				
> static int sp_iop_remove(struct platform_device *pdev)				
> {				
> 	struct sp_iop *iop = iop_poweroff;			
> 	pm_power_off = NULL;			
> 	clk_disable_unprepare(iop->iopclk);			
> 				
> 	return 0;			
> }				

How is it balanced then? remove() is symmetric to probe() but you did
not enable clocks in the probe(). Instead you enable the clocks each
time in load_normal_code() and load_poweroff_code().

This does not seem right at all.

Best regards,
Krzysztof

RE: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021

[Tony Huang 黃懷厚]

Dear Krzysztof:


> >> On 12/03/2022 17:16, Tony Huang wrote:
> >>> This driver is load 8051 bin code.
> >>> The IOP core is DQ8051, so also named IOP8051.
> >>> Need Install DQ8051, The DQ8051 bin file generated by keil C.
> >>> We will provide users with 8051 normal and power off source code.
> >>> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
> >>>
> >>
> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source
> >> How+to+use+I+O+processor+8051+of+-
> >>> files-for-IOP
> >>> Users can follow the operation steps to generate normal.bin and
> >>> poweroff.bin.
> >>> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338
> >>> /26.+IOP8051 26.5?How To Create 8051 bin file
> >>>
> >>> PMC in power management purpose:
> >>> Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> >>> When the power off command is executed.
> >>> The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
> >>> power-off of the system.
> >>>
> >>> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> >>> ---
> >>> Changes in v11:
> >>>  - Addressed comments from Arnd Bergmann.
> >>
> >> How did you address Arnd's comments about splitting the driver to
> >> proper parts? drivers/clk and drivers/power/reset?
> >>
> >
> > drivers/clk : SP7021 system has clock device driver (clk-sp7021.c).
> > So I set the IOP clock through the following function.
> > clk_prepare_enable(iop->iopclk);
> > clk_disable_unprepare(iop->iopclk);
> >
> > drivers/power/reset : SP7021 system does not have a power off device driver.
> 
> What does it mean? The feedback was to split clk and reset features to
> separate drivers and I still see only two patches here with a misc driver. So how
> is his comments addressed? You did not reply in that thread.
> 

I finished replying to Arnd.

> >
> >>>  - Addressed comments from krzysztof.
> >>>
> >>>  MAINTAINERS                |   1 +
> >>>  drivers/misc/Kconfig       |  23 +++
> >>>  drivers/misc/Makefile      |   1 +
> >>>  drivers/misc/sunplus_iop.c | 411
> >>> +++++++++++++++++++++++++++++++++++++++++++++
> >>
> >> The driver looks like SoC specific driver. Why did you put it in
> >> drivers/misc/, not in usual place - drivers/soc/?
> >
> > 8051 is designed for processing I/O events, like receiving IR signal from
> remote controller,
> > taking care of power on or off requests from RTC, or other hardware events
> of external peripherals
> > even when power of main system is off.
> > So I put it in drivers/misc.
> 
> Is IOP8061 a separate device? Your datasheet is saying its embedded in
> SP7021 SoC, so it is a soc driver. This does not fit misc driver description (a
> "strange device") but a SoC driver description.
> 

IOP is a separate device. CPU is 8051.		
SP7021 contains three kinds of CPU.		
Quad-core ARM Cortex-A7 (CA7)		
ARM926 real-time core		
8051 low-power core		

> >
> >> sp_iop_poweroff is still here.
> >
> > sp_iop_poweroff(): SP7021 system does not have a power off device driver.
> > 				I have to put it here.
> 
> This should be rather in a reset driver, not in a misc one. What is your plan for
> this driver? You described the hardware and judging by it, there will be quite a
> lot of separate features so it's reasonable to split the driver into separate
> logical elements. However keeping all in the same place would be ok, if you do
> not plan to add any more features.
> This would mean the driver will handle *only* reset and FW loading.
> 

Can I put sp_iop_poweroff() here for now?				
When power off device driver is added in /driver/power/reset is complete, we will move to it.

> >
> >>>  4 files changed, 436 insertions(+)
> >>>  create mode 100644 drivers/misc/sunplus_iop.c
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS index d64c8ed..c282e95 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -18246,6 +18246,7 @@ SUNPLUS IOP DRIVER
> >>>  M:	Tony Huang <tonyhuang.sunplus@gmail.com>
> >>>  S:	Maintained
> >>>  F:	Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> >>> +F:	drivers/misc/sunplus_iop.c
> >>>
> >>>  SUPERH
> >>>  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
> >>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index
> >>> 0f5a49f..e5f32d8 100644
> >>> --- a/drivers/misc/Kconfig
> >>> +++ b/drivers/misc/Kconfig
> >>> @@ -470,6 +470,29 @@ config HISI_HIKEY_USB
> >>>  	  switching between the dual-role USB-C port and the USB-A host
> ports
> >>>  	  using only one USB controller.
> >>>
> >>> +config SUNPLUS_IOP
> >>> +	tristate "Sunplus IOP support"
> >>> +	default ARCH_SUNPLUS
> >>> +	help
> >>> +	  This driver is load 8051 bin code.
> >>> +	  The IOP core is DQ8051, so also named IOP8051.
> >>> +	  Need Install DQ8051, The DQ8051 bin file generated by keil C.
> >>> +	  We will provide users with 8051 normal and power off source
> code.
> >>> +	  Path:
> >>> +https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
> >>> +
> >>
> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source
> >> How+to+use+I+O+processor+8051+of+-
> >> files-for-IOP
> >>> +	  Users can follow the operation steps to generate normal.bin and
> >> poweroff.bin.
> >>> +	  Path:
> >> https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP
> >> 8051
> >>> +	  26.5?How To Create 8051 bin file
> >>> +
> >>> +	  PMC in power management purpose:
> >>> +	  Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> >>> +	  When the power off command is executed.
> >>> +	  The 8051 has a register(DEF_PWR_EN_0=0) to control the
> power-on
> >> and
> >>> +	  power-off of the system.
> >>> +
> >>> +	  This driver can also be built as a module.  If so, the module
> >>> +	  will be called sunplus_iop.
> >>> +
> >>>  source "drivers/misc/c2port/Kconfig"
> >>>  source "drivers/misc/eeprom/Kconfig"
> >>>  source "drivers/misc/cb710/Kconfig"
> >>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index
> >>> a086197..eafeab6 100644
> >>> --- a/drivers/misc/Makefile
> >>> +++ b/drivers/misc/Makefile
> >>> @@ -52,6 +52,7 @@ obj-$(CONFIG_DW_XDATA_PCIE)	+= dw-xdata-pcie.o
> >>>  obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
> >>>  obj-$(CONFIG_OCXL)		+= ocxl/
> >>>  obj-$(CONFIG_BCM_VK)		+= bcm-vk/
> >>> +obj-$(CONFIG_SUNPLUS_IOP)	+= sunplus_iop.o
> >>>  obj-y				+= cardreader/
> >>>  obj-$(CONFIG_PVPANIC)   	+= pvpanic/
> >>>  obj-$(CONFIG_HABANA_AI)		+= habanalabs/
> >>> diff --git a/drivers/misc/sunplus_iop.c b/drivers/misc/sunplus_iop.c
> >>> new file mode 100644 index 0000000..5bdce5e
> >>> --- /dev/null
> >>> +++ b/drivers/misc/sunplus_iop.c
> >>> @@ -0,0 +1,411 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * The IOP driver for Sunplus SP7021
> >>> + *
> >>> + * Copyright (C) 2021 Sunplus Technology Inc.
> >>> + *
> >>> + * All Rights Reserved.
> >>> + */
> >>> +#include <linux/clk.h>
> >>> +#include <linux/delay.h>
> >>> +#include <linux/dma-mapping.h>
> >>> +#include <linux/firmware.h>
> >>> +#include <linux/iopoll.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/of_platform.h>
> >>> +#include <linux/of_address.h>
> >>> +#include <linux/of_gpio.h>
> >>> +
> >>> +/* IOP register offset */
> >>> +#define IOP_CONTROL	0x00
> >>> +#define IOP_DATA0	0x20
> >>> +#define IOP_DATA1	0x24
> >>> +#define IOP_DATA2	0x28
> >>> +#define IOP_DATA3	0x2c
> >>> +#define IOP_DATA4	0x30
> >>> +#define IOP_DATA5	0x34
> >>> +#define IOP_DATA6	0x38
> >>> +#define IOP_DATA7	0x3c
> >>> +#define IOP_DATA8	0x40
> >>> +#define IOP_DATA9	0x44
> >>> +#define IOP_DATA10	0x48
> >>> +#define IOP_DATA11	0x4c
> >>> +#define IOP_BASE_ADR_L	0x50
> >>> +#define IOP_BASE_ADR_H	0x54
> >>> +
> >>> +/* PMC register offset */
> >>> +#define IOP_PMC_TIMER		0x00
> >>> +#define IOP_PMC_CTRL		0x04
> >>> +#define IOP_XTAL27M_PASSWORD_I	0x08
> >>> +#define IOP_XTAL27M_PASSWORD_II	0x0c
> >>> +#define IOP_XTAL32K_PASSWORD_I	0x10
> >>> +#define IOP_XTAL32K_PASSWORD_II	0x14
> >>> +#define IOP_CLK27M_PASSWORD_I	0x18
> >>> +#define IOP_CLK27M_PASSWORD_II	0x1c
> >>> +#define IOP_PMC_TIMER2		0x20
> >>> +
> >>> +/* Max size of poweroff.bin that can be received */ #define
> >>> +POWEROFF_CODE_MAX_SIZE 0x4000
> >>> +
> >>> +/* 8051 informs linux kerenl. 8051 has been switched to poweroff.bin
> code.
> >> */
> >>> +#define IOP_READY	0x0004
> >>> +#define RISC_READY	0x0008
> >>> +
> >>> +/* System linux kernel tells 8051 which  gpio pin to wake-up through. */
> >>> +#define WAKEUP_PIN	0xFE02
> >>> +
> >>> +/*
> >>> + * There are 3 power domains in SP7021, AO domain, IOP domain,
> >>> + * Default domain. Default domain is linux kernel system.
> >>> + * System linux kernel tells 8051 to execute power off.
> >>> + */
> >>> +#define DEFAULT_DOMAIN_POWEROFF	0x5331 /* AO&IOP domain
> on,
> >> Default domain off */
> >>> +#define DEFAULT_AND_IOP_DOMAIN_POWEROFF	0x5333 /* AO
> domain
> >> on, IOP&Default domain off */
> >>> +
> >>> +struct sp_iop {
> >>> +	struct device *dev;
> >>> +	struct clk *iopclk;
> >>> +	struct reset_control *rstc;
> >>> +	void __iomem *iop_regs;
> >>> +	void __iomem *pmc_regs;
> >>> +	void __iomem *moon0_regs;
> >>> +	int irq;
> >>> +	int gpio_wakeup;
> >>> +	resource_size_t iop_mem_start;
> >>> +	resource_size_t iop_mem_size;
> >>> +	unsigned char bin_code_mode;
> >>> +};
> >>> +
> >>> +static struct sp_iop *iop_poweroff;
> >>> +
> >>> +static void sp_iop_load_normal_code(struct sp_iop *iop) {
> >>> +	const struct firmware *fw;
> >>> +	void __iomem *iop_kernel_base;
> >>> +	unsigned int reg, err;
> >>> +
> >>> +	err = request_firmware(&fw, "normal.bin", iop->dev);
> >>> +	if (err)
> >>> +		dev_err(iop->dev, "get bin file error\n");
> >>> +
> >>> +	iop_kernel_base = ioremap(iop->iop_mem_start, fw->size);
> >>> +	memset(iop_kernel_base, 0, fw->size);
> >>> +	memcpy(iop_kernel_base, fw->data, fw->size);
> >>> +	release_firmware(fw);
> >>> +
> >>> +	clk_prepare_enable(iop->iopclk);
> >>
> >> where do you disable the clock?
> >
> > I will add disable clock in sp_iop_remove().
> > Below is my modification
> > static int sp_iop_remove(struct platform_device *pdev)
> > {
> > 	struct sp_iop *iop = iop_poweroff;
> > 	pm_power_off = NULL;
> > 	clk_disable_unprepare(iop->iopclk);
> >
> > 	return 0;
> > }
> 
> How is it balanced then? remove() is symmetric to probe() but you did not
> enable clocks in the probe(). Instead you enable the clocks each time in
> load_normal_code() and load_poweroff_code().
> 
> This does not seem right at all.

OK, I will modify it.

Re: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021

[Krzysztof Kozlowski]

On 15/03/2022 03:08, Tony Huang 黃懷厚 wrote:
> Dear Krzysztof:
> 
> 
>>>> On 12/03/2022 17:16, Tony Huang wrote:
>>>>> This driver is load 8051 bin code.
>>>>> The IOP core is DQ8051, so also named IOP8051.
>>>>> Need Install DQ8051, The DQ8051 bin file generated by keil C.
>>>>> We will provide users with 8051 normal and power off source code.
>>>>> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
>>>>>
>>>>
>> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source
>>>> How+to+use+I+O+processor+8051+of+-
>>>>> files-for-IOP
>>>>> Users can follow the operation steps to generate normal.bin and
>>>>> poweroff.bin.
>>>>> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338
>>>>> /26.+IOP8051 26.5?How To Create 8051 bin file
>>>>>
>>>>> PMC in power management purpose:
>>>>> Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
>>>>> When the power off command is executed.
>>>>> The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
>>>>> power-off of the system.
>>>>>
>>>>> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
>>>>> ---
>>>>> Changes in v11:
>>>>>  - Addressed comments from Arnd Bergmann.
>>>>
>>>> How did you address Arnd's comments about splitting the driver to
>>>> proper parts? drivers/clk and drivers/power/reset?
>>>>
>>>
>>> drivers/clk : SP7021 system has clock device driver (clk-sp7021.c).
>>> So I set the IOP clock through the following function.
>>> clk_prepare_enable(iop->iopclk);
>>> clk_disable_unprepare(iop->iopclk);
>>>
>>> drivers/power/reset : SP7021 system does not have a power off device driver.
>>
>> What does it mean? The feedback was to split clk and reset features to
>> separate drivers and I still see only two patches here with a misc driver. So how
>> is his comments addressed? You did not reply in that thread.
>>
> 
> I finished replying to Arnd.
> 
>>>
>>>>>  - Addressed comments from krzysztof.
>>>>>
>>>>>  MAINTAINERS                |   1 +
>>>>>  drivers/misc/Kconfig       |  23 +++
>>>>>  drivers/misc/Makefile      |   1 +
>>>>>  drivers/misc/sunplus_iop.c | 411
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>
>>>> The driver looks like SoC specific driver. Why did you put it in
>>>> drivers/misc/, not in usual place - drivers/soc/?
>>>
>>> 8051 is designed for processing I/O events, like receiving IR signal from
>> remote controller,
>>> taking care of power on or off requests from RTC, or other hardware events
>> of external peripherals
>>> even when power of main system is off.
>>> So I put it in drivers/misc.
>>
>> Is IOP8061 a separate device? Your datasheet is saying its embedded in
>> SP7021 SoC, so it is a soc driver. This does not fit misc driver description (a
>> "strange device") but a SoC driver description.
>>
> 
> IOP is a separate device. CPU is 8051.		
> SP7021 contains three kinds of CPU.		
> Quad-core ARM Cortex-A7 (CA7)		
> ARM926 real-time core		
> 8051 low-power core		
> 
>>>
>>>> sp_iop_poweroff is still here.
>>>
>>> sp_iop_poweroff(): SP7021 system does not have a power off device driver.
>>> 				I have to put it here.
>>
>> This should be rather in a reset driver, not in a misc one. What is your plan for
>> this driver? You described the hardware and judging by it, there will be quite a
>> lot of separate features so it's reasonable to split the driver into separate
>> logical elements. However keeping all in the same place would be ok, if you do
>> not plan to add any more features.
>> This would mean the driver will handle *only* reset and FW loading.
>>
> 
> Can I put sp_iop_poweroff() here for now?				
> When power off device driver is added in /driver/power/reset is complete, we will move to it.

Not really, because misc drivers is not a place for power off drivers.
The driver here looks now like responsible only for system power
management of a SoC, so most likely drivers/soc. However it has even
less sense to add some feature here and immediately move it somewhere
more appropriate (instead just add it to the appropriate place).

Your moving of parts of it to drivers/power/reset is now confusing. What
will be left here? Please send entire set not just pieces.


Best regards,
Krzysztof

RE: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021

[Tony Huang 黃懷厚]

Dear Krzysztof:


> >>>> On 12/03/2022 17:16, Tony Huang wrote:
> >>>>> This driver is load 8051 bin code.
> >>>>> The IOP core is DQ8051, so also named IOP8051.
> >>>>> Need Install DQ8051, The DQ8051 bin file generated by keil C.
> >>>>> We will provide users with 8051 normal and power off source code.
> >>>>> Path:
> >>>>> https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
> >>>>>
> >>>>
> >>
> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source
> >>>> How+to+use+I+O+processor+8051+of+-
> >>>>> files-for-IOP
> >>>>> Users can follow the operation steps to generate normal.bin and
> >>>>> poweroff.bin.
> >>>>> Path:
> >>>>> https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338
> >>>>> /26.+IOP8051 26.5?How To Create 8051 bin file
> >>>>>
> >>>>> PMC in power management purpose:
> >>>>> Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> >>>>> When the power off command is executed.
> >>>>> The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on
> >>>>> and power-off of the system.
> >>>>>
> >>>>> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> >>>>> ---
> >>>>> Changes in v11:
> >>>>>  - Addressed comments from Arnd Bergmann.
> >>>>
> >>>> How did you address Arnd's comments about splitting the driver to
> >>>> proper parts? drivers/clk and drivers/power/reset?
> >>>>
> >>>
> >>> drivers/clk : SP7021 system has clock device driver (clk-sp7021.c).
> >>> So I set the IOP clock through the following function.
> >>> clk_prepare_enable(iop->iopclk);
> >>> clk_disable_unprepare(iop->iopclk);
> >>>
> >>> drivers/power/reset : SP7021 system does not have a power off device
> driver.
> >>
> >> What does it mean? The feedback was to split clk and reset features
> >> to separate drivers and I still see only two patches here with a misc
> >> driver. So how is his comments addressed? You did not reply in that thread.
> >>
> >
> > I finished replying to Arnd.
> >
> >>>
> >>>>>  - Addressed comments from krzysztof.
> >>>>>
> >>>>>  MAINTAINERS                |   1 +
> >>>>>  drivers/misc/Kconfig       |  23 +++
> >>>>>  drivers/misc/Makefile      |   1 +
> >>>>>  drivers/misc/sunplus_iop.c | 411
> >>>>> +++++++++++++++++++++++++++++++++++++++++++++
> >>>>
> >>>> The driver looks like SoC specific driver. Why did you put it in
> >>>> drivers/misc/, not in usual place - drivers/soc/?
> >>>
> >>> 8051 is designed for processing I/O events, like receiving IR signal
> >>> from
> >> remote controller,
> >>> taking care of power on or off requests from RTC, or other hardware
> >>> events
> >> of external peripherals
> >>> even when power of main system is off.
> >>> So I put it in drivers/misc.
> >>
> >> Is IOP8061 a separate device? Your datasheet is saying its embedded
> >> in
> >> SP7021 SoC, so it is a soc driver. This does not fit misc driver
> >> description (a "strange device") but a SoC driver description.
> >>
> >
> > IOP is a separate device. CPU is 8051.
> > SP7021 contains three kinds of CPU.
> > Quad-core ARM Cortex-A7 (CA7)
> > ARM926 real-time core
> > 8051 low-power core
> >
> >>>
> >>>> sp_iop_poweroff is still here.
> >>>
> >>> sp_iop_poweroff(): SP7021 system does not have a power off device driver.
> >>> 				I have to put it here.
> >>
> >> This should be rather in a reset driver, not in a misc one. What is
> >> your plan for this driver? You described the hardware and judging by
> >> it, there will be quite a lot of separate features so it's reasonable
> >> to split the driver into separate logical elements. However keeping
> >> all in the same place would be ok, if you do not plan to add any more
> features.
> >> This would mean the driver will handle *only* reset and FW loading.
> >>
> >
> > Can I put sp_iop_poweroff() here for now?
> > When power off device driver is added in /driver/power/reset is complete,
> we will move to it.
> 
> Not really, because misc drivers is not a place for power off drivers.
> The driver here looks now like responsible only for system power management
> of a SoC, so most likely drivers/soc. However it has even less sense to add
> some feature here and immediately move it somewhere more appropriate
> (instead just add it to the appropriate place).
> 
> Your moving of parts of it to drivers/power/reset is now confusing. What will
> be left here? Please send entire set not just pieces.
> 

I may not fully understand your requirements.				
You: What is your plan for this driver? 				
< ----- reset driver? Misc driver?				
You: However keeping all in the same place would be ok, if you do not plan to add any more features.				
This would mean the driver will handle *only* reset and FW loading.				
< ------ 	 Because system does not have a power off device driver.			
	sp_iop_poweroff() can be placed in iop driver?			

Re: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021

[Arnd Bergmann]

On Tue, Mar 15, 2022 at 9:24 AM Tony Huang 黃懷厚 <tony.huang@sunplus.com> wrote:
> > >>>> On 12/03/2022 17:16, Tony Huang wrote:

(note: please use normal quoting style, only quote the parts of the message
 that you actually reply to, reave the attributoons in place, etc)

> > > Can I put sp_iop_poweroff() here for now?
> > > When power off device driver is added in /driver/power/reset is complete,
> > we will move to it.
> >
> > Not really, because misc drivers is not a place for power off drivers.
> > The driver here looks now like responsible only for system power management
> > of a SoC, so most likely drivers/soc. However it has even less sense to add
> > some feature here and immediately move it somewhere more appropriate
> > (instead just add it to the appropriate place).
> >
> > Your moving of parts of it to drivers/power/reset is now confusing. What will
> > be left here? Please send entire set not just pieces.
> >
>
> I may not fully understand your requirements.
> You: What is your plan for this driver?
> < ----- reset driver? Misc driver?
> You: However keeping all in the same place would be ok, if you do not plan to add any more features.
> This would mean the driver will handle *only* reset and FW loading.
> < ------         Because system does not have a power off device driver.
>         sp_iop_poweroff() can be placed in iop driver?


It really depends on where you want this driver to head. If you don't
have any plans to add features beyond poweroff, having the whole
thing in drivers/power/reset/ is the easiest way. If you do have plans
to add other features, then explain exactly what those features are,
which subsystems they go into, and why you haven't already
implemented them in the previous 11 versions.

The common bits of the driver can go into drivers/soc or drivers/mfd,
partly depending on what abstraction you use between this
driver and its clients, so it's important to figure out the correct
abstraction layer first, and then decide where it should go.

One common way to do it is to use the multi-function-device
abstraction to automatically create child devices from the
hardware device node. Another option is to have high-level
functions exported from a soc driver. Or you could just have
a 'regmap' exposed from a 'syscon' driver to provide
individual registers to devices. Which one of those is best
for your system mostly depends on how you end up using
it, and we have no idea until you have more than one leaf
driver on it. The poweroff driver is not a great example either,
because it does not have an interesting interface and just
comes down to a single function call.

      Arnd