Skip to contentSkip to conversation

Add iop driver for Sunplus SP7021

PATCH Superseded Tony Huang 5 participants 18 messages Dec 3, 2021 – Dec 6, 2021

Subsystems (2)
Device tree bindings Char and misc drivers

v2 of 12 Superseded Latest is v12 →

lore
Share & tools
lore

Tony Huang

author
#0 lore raw

v2 cover Add iop driver for Sunplus SP7021

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-binding: misc: Add iop yaml file for Sunplus SP7021
misc: Add iop driver for Sunplus SP7021

 .../devicetree/bindings/misc/sunplus-iop.yaml      |  61 +++
 MAINTAINERS                                        |   6 +
 drivers/misc/Kconfig                               |   1 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/iop/Kconfig                           |  13 +
 drivers/misc/iop/Makefile                          |   6 +
 drivers/misc/iop/sunplus_iop.c                     | 518 +++++++++++++++++++++
 drivers/misc/iop/sunplus_iop.h                     |  64 +++
 8 files changed, 670 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/sunplus-iop.yaml
 create mode 100644 drivers/misc/iop/Kconfig
 create mode 100644 drivers/misc/iop/Makefile
 create mode 100644 drivers/misc/iop/sunplus_iop.c
 create mode 100644 drivers/misc/iop/sunplus_iop.h
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH v2 1/2 dt-binding: misc: Add iop yaml file for Sunplus SP7021

Add iop yaml file for Sunplus SP7021

Signed-off-by Tony Huang <[email protected]>

---
Changes in v2:
- Added moon0 register in yaml file.

 .../devicetree/bindings/misc/sunplus-iop.yaml      | 61 ++++++++++++++++++++++
 MAINTAINERS                                        |  5 ++
 2 files changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/sunplus-iop.yaml
Documentation/devicetree/bindings/misc/sunplus-iop.yaml new file +61 -0
new file mode 100644
index 0000000..07cdbde
@@ -0,0 +1,61 @@
+# 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 <[email protected]>
+
+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:
+    maxItems: 2
+
+  memory-region:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - memory-region
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    iop: iop@9c000400 {
+        compatible = "sunplus,sp7021-iop";
+        reg = <0x9c000400 0x80>, <0x9c003100 0x80>, <0x9c000000 0x80>;
+        reg-names = "iop", "iop_pmc", "moon0";
+        interrupt-parent = <&intc>;
+        interrupts = <41 IRQ_TYPE_LEVEL_HIGH>, <42 IRQ_TYPE_LEVEL_HIGH>;
+        memory-region = <&iop_reserve>;
+    };
+...
MAINTAINERS +5 -0
index 3b79fd4..071b5e6 100644
@@ -17945,6 +17945,11 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS IOP DRIVER
+M:	Tony Huang <[email protected]>
+S:	Maintained
+F:	Documentation/devicetree/bindings/misc/sunplu-iop.yaml
+
 SUPERH
 M:	Yoshinori Sato <[email protected]>
 M:	Rich Felker <[email protected]>
Signature · 1 line
2.7.4
author
#2 lore raw

PATCH v2 2/2 misc: Add iop driver for Sunplus SP7021

In reply to Tony Huang

IOP (IO Processor) embedded inside SP7021 which is used as Processor for I/O control, RTC wake-up and cooperation with CPU & PMC in power management purpose.
The IOP core is DQ8051, so also named IOP8051,
it supports dedicated JTAG debug pins which share with SP7021. In standby mode operation, the power spec reach 400uA.

Signed-off-by Tony Huang <[email protected]>

---
Changes in v2:
- Addressed comments from Arnd Bergmann.
- Addressed comments from Greg KH.
- Addressed comments from kernel test robot.

 MAINTAINERS                    |   1 +
 drivers/misc/Kconfig           |   1 +
 drivers/misc/Makefile          |   1 +
 drivers/misc/iop/Kconfig       |  13 ++
 drivers/misc/iop/Makefile      |   6 +
 drivers/misc/iop/sunplus_iop.c | 518 +++++++++++++++++++++++++++++++++++++++++
 drivers/misc/iop/sunplus_iop.h |  64 +++++
 7 files changed, 604 insertions(+)
 create mode 100644 drivers/misc/iop/Kconfig
 create mode 100644 drivers/misc/iop/Makefile
 create mode 100644 drivers/misc/iop/sunplus_iop.c
 create mode 100644 drivers/misc/iop/sunplus_iop.h
MAINTAINERS +1 -0
index 071b5e6..f14aa19 100644
@@ -17949,6 +17949,7 @@ SUNPLUS IOP DRIVER
 M:	Tony Huang <[email protected]>
 S:	Maintained
 F:	Documentation/devicetree/bindings/misc/sunplu-iop.yaml
+F:	drivers/misc/iop
 
 SUPERH
 M:	Yoshinori Sato <[email protected]>
drivers/misc/Kconfig +1 -0
index 0f5a49f..8fbd9a6 100644
@@ -481,6 +481,7 @@ source "drivers/misc/vmw_vmci/Kconfig"
 source "drivers/misc/genwqe/Kconfig"
 source "drivers/misc/echo/Kconfig"
 source "drivers/misc/cxl/Kconfig"
+source "drivers/misc/iop/Kconfig"
 source "drivers/misc/ocxl/Kconfig"
 source "drivers/misc/bcm-vk/Kconfig"
 source "drivers/misc/cardreader/Kconfig"
drivers/misc/Makefile +1 -0
index a086197..5ab7fb0 100644
@@ -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)	+= iop/
 obj-y				+= cardreader/
 obj-$(CONFIG_PVPANIC)   	+= pvpanic/
 obj-$(CONFIG_HABANA_AI)		+= habanalabs/
drivers/misc/iop/Kconfig new file +13 -0
new file mode 100644
index 0000000..3afda5c
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# IOP configuration
+#
+
+config SUNPLUS_IOP
+	tristate "Sunplus IOP support"
+	default ARCH_SUNPLUS
+	help
+	  Sunplus I/O processor (8051) driver.
+	  Processor for I/O control, RTC wake-up proceduce management,
+	  and cooperation with CPU&PMC in power management.
+	  Need Install DQ8051, The DQ8051 bin file generated by keil C.
\ No newline at end of file
drivers/misc/iop/Makefile new file +6 -0
new file mode 100644
index 0000000..e44b021
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for the IOP module drivers.
+#
+
+obj-$(CONFIG_SUNPLUS_IOP) += sunplus_iop.o
drivers/misc/iop/sunplus_iop.c new file +518 -0

518 lines not on this page · raw message

drivers/misc/iop/sunplus_iop.h new file +64 -0

64 lines not on this page · raw message

Signature · 1 line
2.7.4
build bot
#3 lore raw

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

I love your patch! Yet something to improve:

Hi Tony,

I love your patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on robh/for-next soc/for-next linus/master v5.16-rc3 next-20211202] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tony-Huang/Add-iop-driver-for-Sunplus-SP7021/20211203-114932
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 5d331b5922551637c586cdf5fdc1778910fc937f
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20211203/202112031753.KIjg9ffN-lkp(a)intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/5a13966416937e820ad198a487deb9308cb86061
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tony-Huang/Add-iop-driver-for-Sunplus-SP7021/20211203-114932 git checkout 5a13966416937e820ad198a487deb9308cb86061
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

In file included from drivers/misc/iop/sunplus_iop.c:10:

>> drivers/misc/iop/sunplus_iop.h:5:10: fatal error: mach/io_map.h: No such file or directory
       5 | #include <mach/io_map.h>
         |          ^~~~~~~~~~~~~~~

compilation terminated.

vim +5 drivers/misc/iop/sunplus_iop.h

     2	
     3	#ifndef __SP_IOP_H__
     4	#define __SP_IOP_H__
   > 5	#include <mach/io_map.h>
     6

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

maintainer
#4 lore raw

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

In reply to Tony Huang

drivers/misc/iop/sunplus_iop.c line 15

Quoted 4 lines · Tony Huang
drivers/misc/iop/sunplus_iop.c new file +4 -0
+#define NORMAL_CODE_MAX_SIZE 0X1000
+#define STANDBY_CODE_MAX_SIZE 0x4000
+unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE];
+unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE];

Please make your local variables static so that yhou do not polute the kernel's global symbol table.

drivers/misc/iop/sunplus_iop.c line 19

Quoted 4 lines
drivers/misc/iop/sunplus_iop.c new file +4 -0
+/* ---------------------------------------------------------------------------------------------- */
+resource_size_t SP_IOP_RESERVE_BASE;
+resource_size_t SP_IOP_RESERVE_SIZE;
+/* ---------------------------------------------------------------------------------------------- */

Again, static.

And why the odd comment lines?

And those are not good variable names.

drivers/misc/iop/sunplus_iop.c line 30

Quoted 11 lines
drivers/misc/iop/sunplus_iop.c new file +11 -0
+struct sp_iop {
+	struct miscdevice dev;			// iop device
+	struct mutex write_lock;
+	void __iomem *iop_regs;
+	void __iomem *pmc_regs;
+	void __iomem *moon0_regs;
+	int irq;
+};
+/*****************************************************************
+ *						  G L O B A L	 D A T A
+ ******************************************************************/

Global where? What about the ones above? :)

Quoted 1 line

+static struct sp_iop *iop;

Why do you think you only have one device in the system? Please do not use a single variable like this. It is easy to make your driver handle an unlimited number of devices just as easy as to handle 1 device. Please do that instead and hang your device-specific data off of the correct data structures that the driver core gives you.

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+void iop_normal_mode(void)

Did you run sparse on this code? Please do so.

Also, no need for a .h file for a driver that only has one .c file.

thanks,

greg k-h

#5 lore raw

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

Dear Greg KH:

Quoted 10 lines

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Friday, December 3, 2021 6:39 PM
To: Tony Huang <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Wells Lu 呂芳騰 <[email protected]>; Tony Huang 黃懷厚 <[email protected]>
Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

Quoted 9 lines

drivers/misc/iop/sunplus_iop.c line 15

Quoted 4 lines · Tony Huang
drivers/misc/iop/sunplus_iop.c new file +4 -0
+#define NORMAL_CODE_MAX_SIZE 0X1000
+#define STANDBY_CODE_MAX_SIZE 0x4000
+unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE];
+unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE];

Please make your local variables static so that yhou do not polute the kernel's global symbol table.

I will modify it.

Quoted 10 lines

Review

Quoted 7 lines
(quoted diff) +7 -0
+/*
+---------------------------------------------------------------------
+------------------------- */ resource_size_t SP_IOP_RESERVE_BASE;
+resource_size_t SP_IOP_RESERVE_SIZE;
+/*
+---------------------------------------------------------------------
+------------------------- */

Again, static.

Quoted 2 lines

And why the odd comment lines?

I will remove it.

Quoted 13 lines

And those are not good variable names.

drivers/misc/iop/sunplus_iop.c line 27

Quoted 9 lines
drivers/misc/iop/sunplus_iop.c new file +8 -0
+struct sp_iop {
+	struct miscdevice dev;			// iop device
+	struct mutex write_lock;
+	void __iomem *iop_regs;
+	void __iomem *pmc_regs;
+	void __iomem *moon0_regs;
+	int irq;
+};

+/**************************************************************
***

Quoted 7 lines
Quoted 2 lines

+ * G L O B A L D A T A
+

****************************************************************
**/

Global where? What about the ones above? :)

I will remove it.

Quoted 8 lines
Quoted 1 line

+static struct sp_iop *iop;

Why do you think you only have one device in the system? Please do not use a single variable like this. It is easy to make your driver handle an unlimited number of devices just as easy as to handle 1 device.
Please do that instead and hang your device-specific data off of the correct data structures that the driver core gives you.

I will modify it.

Quoted 7 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+void iop_normal_mode(void)

Did you run sparse on this code? Please do so.

Also, no need for a .h file for a driver that only has one .c file.

I need to keep sunplus_iop.h. Other files will use sp_iop_platform_driver_poweroff(void) in poweroff flow.

Quoted 3 lines

thanks,

greg k-h

maintainer
#6 lore raw

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

Quoted 5 lines · Tony Huang 黃懷厚
Quoted 2 lines

Also, no need for a .h file for a driver that only has one .c file.

I need to keep sunglus_iop.h. Other files will use sp_iop_platform_driver_poweroff(void) in poweroff flow.

What other files? That is not included here, nor should other drivers be making that call, use the normal poweroff logic.

thanks,

greg k-h

#7 lore raw

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

Dear Greg KH:

Quoted 9 lines
Quoted 5 lines
Quoted 2 lines

Also, no need for a .h file for a driver that only has one .c file.

I need to keep sunglus_iop.h. Other files will use sp_iop_platform_driver_poweroff(void) in poweroff flow.

What other files? That is not included here, nor should other drivers be making that call, use the normal poweroff logic.

Okay, I will call the sp_iop_platform_driver_poweroff(void) through normal poweroff logic.

maintainer
#8 lore raw

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

In reply to Tony Huang
Quoted 14 lines · Tony Huang

IOP (IO Processor) embedded inside SP7021 which is used as Processor for I/O control, RTC wake-up and cooperation with CPU & PMC in power management purpose.
The IOP core is DQ8051, so also named IOP8051,
it supports dedicated JTAG debug pins which share with SP7021. In standby mode operation, the power spec reach 400uA.

Signed-off-by Tony Huang <[email protected]>

---
Changes in v2:
- Addressed comments from Arnd Bergmann.
- Addressed comments from Greg KH.
- Addressed comments from kernel test robot.

Quoted 5 lines
 MAINTAINERS                    |   1 +
 drivers/misc/Kconfig           |   1 +
 drivers/misc/Makefile          |   1 +
 drivers/misc/iop/Kconfig       |  13 ++
 drivers/misc/iop/Makefile      |   6 +

Why do you need a subdirectory for a single .c file?

thanks,

greg k-h

#9 lore raw

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

Dear Greg KH, Arnd:

Quoted 25 lines

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Friday, December 3, 2021 6:39 PM
To: Tony Huang <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Wells Lu 呂芳騰 <[email protected]>; Tony Huang 黃懷厚 <[email protected]>
Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

Quoted 14 lines · Tony Huang

IOP (IO Processor) embedded inside SP7021 which is used as Processor for I/O control, RTC wake-up and cooperation with CPU & PMC in power management purpose.
The IOP core is DQ8051, so also named IOP8051, it supports dedicated JTAG debug pins which share with SP7021.
In standby mode operation, the power spec reach 400uA.

Signed-off-by Tony Huang <[email protected]>

---
Changes in v2:
- Addressed comments from Arnd Bergmann.
- Addressed comments from Greg KH.
- Addressed comments from kernel test robot.

Quoted 8 lines
Quoted 5 lines
 MAINTAINERS                    |   1 +
 drivers/misc/Kconfig           |   1 +
 drivers/misc/Makefile          |   1 +
 drivers/misc/iop/Kconfig       |  13 ++
 drivers/misc/iop/Makefile      |   6 +

Why do you need a subdirectory for a single .c file?

1. Currently my bin file is placed in the root file system. I need to wait for the kernel booting to succeed before loading the bin code.
2. In addition, I need the kernel booting process to be able to mount the iop module and load bin file.I need to put bin file in /iop. Do you have a good way to load bin code during kernel booting?

Thanks

maintainer
#10 lore raw

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

Quoted 33 lines · Tony Huang 黃懷厚

Dear Greg KH, Arnd:

Quoted 31 lines

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Friday, December 3, 2021 6:39 PM
To: Tony Huang <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Wells Lu 呂芳騰 <[email protected]>; Tony Huang 黃懷厚 <[email protected]>
Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

Quoted 19 lines · Tony Huang

IOP (IO Processor) embedded inside SP7021 which is used as Processor for I/O control, RTC wake-up and cooperation with CPU & PMC in power management purpose.
The IOP core is DQ8051, so also named IOP8051, it supports dedicated JTAG debug pins which share with SP7021.
In standby mode operation, the power spec reach 400uA.

Signed-off-by Tony Huang <[email protected]>

---
Changes in v2:
- Addressed comments from Arnd Bergmann.
- Addressed comments from Greg KH.
- Addressed comments from kernel test robot.

 MAINTAINERS                    |   1 +
 drivers/misc/Kconfig           |   1 +
 drivers/misc/Makefile          |   1 +
 drivers/misc/iop/Kconfig       |  13 ++
 drivers/misc/iop/Makefile      |   6 +
Quoted 5 lines
Quoted 2 lines

Why do you need a subdirectory for a single .c file?

1. Currently my bin file is placed in the root file system. I need to wait for the kernel booting to succeed before loading the bin code.

What "bin file"?

Quoted 2 lines

2. In addition, I need the kernel booting process to be able to mount the iop module and load bin file.I need to put bin file in /iop.

That is an odd directory location.

Quoted 1 line

Do you have a good way to load bin code during kernel booting?

Is this firmware? Put it in the normal location for firware that the kernel expects to see.

thanks,

greg k-h

#11 lore raw

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

Dear Greg KH:

Quoted 45 lines

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Monday, December 6, 2021 4:07 PM
To: Tony Huang 黃懷厚 <[email protected]>
Cc: Tony Huang <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Wells Lu 呂芳騰 <[email protected]>
Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

Quoted 34 lines · Tony Huang 黃懷厚

Dear Greg KH, Arnd:

Quoted 32 lines

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Friday, December 3, 2021 6:39 PM
To: Tony Huang <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Wells Lu 呂芳騰 <[email protected]>; Tony Huang 黃懷厚
<[email protected]>
Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

Quoted 19 lines · Tony Huang

IOP (IO Processor) embedded inside SP7021 which is used as Processor for I/O control, RTC wake-up and cooperation with CPU & PMC in power management purpose.
The IOP core is DQ8051, so also named IOP8051, it supports dedicated JTAG debug pins which share with SP7021.
In standby mode operation, the power spec reach 400uA.

Signed-off-by Tony Huang <[email protected]>

---
Changes in v2:
- Addressed comments from Arnd Bergmann.
- Addressed comments from Greg KH.
- Addressed comments from kernel test robot.

 MAINTAINERS                    |   1 +
 drivers/misc/Kconfig           |   1 +
 drivers/misc/Makefile          |   1 +
 drivers/misc/iop/Kconfig       |  13 ++
 drivers/misc/iop/Makefile      |   6 +
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Why do you need a subdirectory for a single .c file?

1. Currently my bin file is placed in the root file system. I need to wait for the kernel booting to succeed before loading the bin code.

What "bin file"?

IOP MODULE EXECUTES 8051 CODE
Source code should reserve SDRAM memory area for IOP module code. 8051 bin file normal code and standby code can be placed in this area. The location area can be select by user. Normal code: Monitor CPU commands.
Standby code: For RTC wake up, cooperation with CPU&PMC in power management When the system enters standby mode, 8051 bin file should be moved to I_Cache. I_Cache has 16K only. Standby code cannot exceed 16K.
When the IOP module is mounted, CPU load 8051 codes (normal.bin) into memory.
Iop_base_addr_l and iop_base_addr_h specify address.
During system boot up, when the IOP is mounted, it will load 8051 normal code and start execute 8051 code.

Quoted 13 lines
Quoted 2 lines

2. In addition, I need the kernel booting process to be able to mount the iop module and load bin file.I need to put bin file in /iop.

That is an odd directory location.

Quoted 1 line

Do you have a good way to load bin code during kernel booting?

Is this firmware? Put it in the normal location for firware that the kernel expects to see.

thanks,

greg k-h

maintainer
#12 lore raw

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

Quoted 56 lines · Tony Huang 黃懷厚

Dear Greg KH:

Quoted 45 lines

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Monday, December 6, 2021 4:07 PM
To: Tony Huang 黃懷厚 <[email protected]>
Cc: Tony Huang <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Wells Lu 呂芳騰 <[email protected]>
Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

Quoted 34 lines · Tony Huang 黃懷厚

Dear Greg KH, Arnd:

Quoted 32 lines

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Friday, December 3, 2021 6:39 PM
To: Tony Huang <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Wells Lu 呂芳騰 <[email protected]>; Tony Huang 黃懷厚
<[email protected]>
Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

Quoted 19 lines · Tony Huang

IOP (IO Processor) embedded inside SP7021 which is used as Processor for I/O control, RTC wake-up and cooperation with CPU & PMC in power management purpose.
The IOP core is DQ8051, so also named IOP8051, it supports dedicated JTAG debug pins which share with SP7021.
In standby mode operation, the power spec reach 400uA.

Signed-off-by Tony Huang <[email protected]>

---
Changes in v2:
- Addressed comments from Arnd Bergmann.
- Addressed comments from Greg KH.
- Addressed comments from kernel test robot.

 MAINTAINERS                    |   1 +
 drivers/misc/Kconfig           |   1 +
 drivers/misc/Makefile          |   1 +
 drivers/misc/iop/Kconfig       |  13 ++
 drivers/misc/iop/Makefile      |   6 +
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Why do you need a subdirectory for a single .c file?

1. Currently my bin file is placed in the root file system. I need to wait for the kernel booting to succeed before loading the bin code.

What "bin file"?

Quoted 8 lines

IOP MODULE EXECUTES 8051 CODE
Source code should reserve SDRAM memory area for IOP module code. 8051 bin file normal code and standby code can be placed in this area. The location area can be select by user. Normal code: Monitor CPU commands.
Standby code: For RTC wake up, cooperation with CPU&PMC in power management When the system enters standby mode, 8051 bin file should be moved to I_Cache. I_Cache has 16K only. Standby code cannot exceed 16K.
When the IOP module is mounted, CPU load 8051 codes (normal.bin) into memory.
Iop_base_addr_l and iop_base_addr_h specify address.
During system boot up, when the IOP is mounted, it will load 8051 normal code and start execute 8051 code.

So this is firmware, just put it in the normal firmware location.

thanks,

greg k-h

maintainer
#13 lore raw

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

In reply to Tony Huang

drivers/misc/iop/sunplus_iop.c line 1

Quoted 4 lines · Tony Huang
drivers/misc/iop/sunplus_iop.c new file +1 -0
@@ -0,0 +1,518 @@
+// SPDX-License-Identifier: GPL-2.0-or-later

Are you sure about "or later"? I have to ask.

Also, no copyright information on the file? Are you sure about that?

thanks,

greg k-h

maintainer
#14 lore raw

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

In reply to Tony Huang
Quoted 8 lines · Tony Huang

IOP (IO Processor) embedded inside SP7021 which is used as Processor for I/O control, RTC wake-up and cooperation with CPU & PMC in power management purpose.
The IOP core is DQ8051, so also named IOP8051,
it supports dedicated JTAG debug pins which share with SP7021. In standby mode operation, the power spec reach 400uA.

Quoted 6 lines
Signed-off-by Tony Huang <[email protected]>

---
Changes in v2:
- Addressed comments from Arnd Bergmann.
- Addressed comments from Greg KH.
- Addressed comments from kernel test robot.

This looks much better already.

drivers/misc/iop/sunplus_iop.c line 15

Quoted 4 lines
drivers/misc/iop/sunplus_iop.c new file +4 -0
+#define NORMAL_CODE_MAX_SIZE 0X1000
+#define STANDBY_CODE_MAX_SIZE 0x4000
+unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE];
+unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE];

I think these should be part of the sp_iop structure, not global variables.

drivers/misc/iop/sunplus_iop.c line 35

Quoted 5 lines
drivers/misc/iop/sunplus_iop.c new file +5 -0
+static struct sp_iop *iop;
+
+void iop_normal_mode(void)
+{
+       struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;

local functions should generally be 'static' and take the 'struct sp_iop' instance
pointer as the first argument.

Functions that need to be global because they are used by other drivers (if any) should probably also be exported and have documentation above their definition.

drivers/misc/iop/sunplus_iop.c line 304

Quoted 8 lines
drivers/misc/iop/sunplus_iop.c new file +8 -0
+static int  get_normal_code(struct device *dev)
+{
+       const struct firmware *fw;
+       static const char file[] = "normal.bin";
+       unsigned int err, i;
+
+       dev_info(dev, "normal code\n");
+       err = request_firmware(&fw, file, dev);

The file name needs to clearly identify the device to avoid conflicts with other drivers.

Quoted 20 lines
drivers/misc/iop/sunplus_iop.c new file +20 -0
+static int sp_iop_open(struct inode *inode, struct file *pfile)
+{
+       return 0;
+}
+
+static ssize_t sp_iop_read(struct file *pfile, char __user *ubuf,
+                       size_t length, loff_t *offset)
+{
+       return 0;
+}
+
+static ssize_t sp_iop_write(struct file *pfile, const char __user *ubuf,
+       size_t length, loff_t *offset)
+{
+       return 0;
+}
+
+static int sp_iop_release(struct inode *inode, struct file *pfile)
+{
+       //dev_dbg(iop->dev, "Sunplus IOP module release\n");

drivers/misc/iop/sunplus_iop.c line 372

Quoted 10 lines
drivers/misc/iop/sunplus_iop.c new file +10 -0
+       return 0;
+}
+
+static const struct file_operations sp_iop_fops = {
+       .owner                  = THIS_MODULE,
+       .open                   = sp_iop_open,
+       .read                   = sp_iop_read,
+       .write                  = sp_iop_write,
+       .release                = sp_iop_release,
+};

This does nothing because all the callbacks are empty. You removed the inappropriate
user space interfaces as I asked you to, but if there is no way for either kernel or user
space to interact with the hardware, I don't see a point in merging the driver until
you add a new interface that is usable.

Quoted 10 lines
drivers/misc/iop/sunplus_iop.c new file +10 -0
+static int sp_iop_platform_driver_remove(struct platform_device *pdev)
+{
+       return 0;
+}
+
+static int sp_iop_platform_driver_suspend(struct platform_device *pdev, pm_message_t state)
+{
+       return 0;
+}
+

drivers/misc/iop/sunplus_iop.c line 487

Quoted 10 lines
drivers/misc/iop/sunplus_iop.c new file +10 -0
+static void sp_iop_platform_driver_shutdown(struct platform_device *pdev)
+{
+
+}
+
+void sp_iop_platform_driver_poweroff(void)
+{
+       iop_standby_mode();
+       iop_shutdown();
+}

Something looks wrong here, maybe reread the documentation for runtime power management
to find a way of putting the device into low-power mode when it is unused.

drivers/misc/iop/sunplus_iop.h line 5

Quoted 11 lines
drivers/misc/iop/sunplus_iop.h new file +5 -0
new file mode 100644
index 0000000..fcbfd26
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later*/
+
+#ifndef __SP_IOP_H__
+#define __SP_IOP_H__
+#include <mach/io_map.h>

mach/io_map.h does not exist, so the driver won't compile. I don't think you need anything else, so it should be fine to remove the #include

The rest of the header only describes the hardware itself, so I'd suggest merging
all of it into the .c file.

Arnd

#15 lore raw

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

Dear Arnd:

Quoted 19 lines

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

Quoted 8 lines · Tony Huang

IOP (IO Processor) embedded inside SP7021 which is used as Processor for I/O control, RTC wake-up and cooperation with CPU & PMC in power management purpose.
The IOP core is DQ8051, so also named IOP8051, it supports dedicated JTAG debug pins which share with SP7021.
In standby mode operation, the power spec reach 400uA.

Quoted 6 lines
Signed-off-by Tony Huang <[email protected]>

---
Changes in v2:
- Addressed comments from Arnd Bergmann.
- Addressed comments from Greg KH.
- Addressed comments from kernel test robot.

Quoted 9 lines

This looks much better already.

drivers/misc/iop/sunplus_iop.c line 15

Quoted 4 lines
drivers/misc/iop/sunplus_iop.c new file +4 -0
+#define NORMAL_CODE_MAX_SIZE 0X1000
+#define STANDBY_CODE_MAX_SIZE 0x4000
+unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE];
+unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE];

I think these should be part of the sp_iop structure, not global variables.

I will modify it.

Quoted 9 lines

drivers/misc/iop/sunplus_iop.c line 35

Quoted 5 lines
drivers/misc/iop/sunplus_iop.c new file +5 -0
+static struct sp_iop *iop;
+
+void iop_normal_mode(void)
+{
+       struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;

local functions should generally be 'static' and take the 'struct sp_iop' instance pointer as the first argument.

Quoted 4 lines

Functions that need to be global because they are used by other drivers (if any) should probably also be exported and have documentation above their definition.

I will mocify it.

Quoted 11 lines

drivers/misc/iop/sunplus_iop.c line 304

Quoted 7 lines
drivers/misc/iop/sunplus_iop.c new file +7 -0
+static int  get_normal_code(struct device *dev) {
+       const struct firmware *fw;
+       static const char file[] = "normal.bin";
+       unsigned int err, i;
+
+       dev_info(dev, "normal code\n");
+       err = request_firmware(&fw, file, dev);

The file name needs to clearly identify the device to avoid conflicts with other drivers.

I will moidfy it.

Quoted 23 lines
Quoted 23 lines
drivers/misc/iop/sunplus_iop.c new file +23 -0
+static int sp_iop_open(struct inode *inode, struct file *pfile) {
+       return 0;
+}
+
+static ssize_t sp_iop_read(struct file *pfile, char __user *ubuf,
+                       size_t length, loff_t *offset) {
+       return 0;
+}
+
+static ssize_t sp_iop_write(struct file *pfile, const char __user *ubuf,
+       size_t length, loff_t *offset) {
+       return 0;
+}
+
+static int sp_iop_release(struct inode *inode, struct file *pfile) {
+       //dev_dbg(iop->dev, "Sunplus IOP module release\n");
+       return 0;
+}
+
+static const struct file_operations sp_iop_fops = {
+       .owner                  = THIS_MODULE,
+       .open                   = sp_iop_open,
+       .read                   = sp_iop_read,
Quoted 9 lines

drivers/misc/iop/sunplus_iop.c line 372

Quoted 3 lines
drivers/misc/iop/sunplus_iop.c new file +3 -0
+       .write                  = sp_iop_write,
+       .release                = sp_iop_release,
+};

This does nothing because all the callbacks are empty. You removed the inappropriate user space interfaces as I asked you to, but if there is no way for either kernel or user space to interact with the hardware, I don't see a point in merging the driver until you add a new interface that is usable.

I will modify sp_iop_read() to monitor IOP mailbox data.

Quoted 16 lines
Quoted 16 lines
drivers/misc/iop/sunplus_iop.c new file +16 -0
+static int sp_iop_platform_driver_remove(struct platform_device
+*pdev) {
+       return 0;
+}
+
+static int sp_iop_platform_driver_suspend(struct platform_device
+*pdev, pm_message_t state) {
+       return 0;
+}
+
+static void sp_iop_platform_driver_shutdown(struct platform_device
+*pdev) {
+
+}
+
+void sp_iop_platform_driver_poweroff(void)
Quoted 9 lines

drivers/misc/iop/sunplus_iop.c line 487

Quoted 4 lines
drivers/misc/iop/sunplus_iop.c new file +4 -0
+{
+       iop_standby_mode();
+       iop_shutdown();
+}

Something looks wrong here, maybe reread the documentation for runtime power management to find a way of putting the device into low-power mode when it is unused.

When the poweroff command is executed, the run sp_iop_platform_driver_poweroff(void) function will enter the standby mode. The power off will be executed. In the system, IOP can continue to work when other modules in the system enter standby / power down modes to monitor whether the system wakes up through RTC.

26.4.2 IOP MODULE EXECUTES 8051 CODE
Source code should reserve SDRAM memory area for IOP module code. 8051 bin file normal code and standby code can be placed in this area. The location area can be select by user. Normal code: Monitor CPU commands.
Standby code: For RTC wake up, cooperation with CPU&PMC in power management
When the system enters standby mode, 8051 bin file should be moved to I_Cache.
I_Cache has 16K only. Standby code cannot exceed 16K.
When the IOP module is mounted, CPU load 8051 codes (normal.bin) into memory.
Iop_base_addr_l and iop_base_addr_h specify address.
During system boot up, when the IOP is mounted, it will load 8051 normal code and start execute 8051 code.

In the system, IOP can continue to work when other modules in the system enter standby / power down modes to monitor whether the system wakes up through RTC. In such a situation, the IOP will not be able to access the system memory.

Quoted 6 lines
Quoted 6 lines
(quoted diff) +0 -0

b/drivers/misc/iop/sunplus_iop.h new file mode 100644 index 0000000..fcbfd26

drivers/misc/iop/sunplus_iop.h new file +0 -0
@@ -0,0 +1,64 @@
Quoted 9 lines

drivers/misc/iop/sunplus_iop.h line 5

Quoted 5 lines
drivers/misc/iop/sunplus_iop.h new file +5 -0
+/* SPDX-License-Identifier: GPL-2.0-or-later*/
+
+#ifndef __SP_IOP_H__
+#define __SP_IOP_H__
+#include <mach/io_map.h>

mach/io_map.h does not exist, so the driver won't compile. I don't think you need anything else, so it should be fine to remove the #include

I will remove io_map.h file.

Quoted 3 lines

The rest of the header only describes the hardware itself, so I'd suggest merging all of it into the .c file.

I need to keep sunplus_iop.h. Other files will use sp_iop_platform_driver_poweroff(void) in poweroff flow.
When the poweroff command is executed, the run sp_iop_platform_driver_poweroff(void) function will enter the standby mode. The power off will be executed.

maintainer
#16 lore raw

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

Quoted 17 lines · Tony Huang 黃懷厚
Quoted 16 lines

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

drivers/misc/iop/sunplus_iop.c line 372

Quoted 8 lines · Tony Huang
drivers/misc/iop/sunplus_iop.c new file +8 -0
+
+static const struct file_operations sp_iop_fops = {
+       .owner                  = THIS_MODULE,
+       .open                   = sp_iop_open,
+       .read                   = sp_iop_read,
+       .write                  = sp_iop_write,
+       .release                = sp_iop_release,
+};

This does nothing because all the callbacks are empty. You removed the inappropriate user space interfaces as I asked you to, but if there is no way for either kernel or user space to interact with the hardware, I don't see a point in merging the driver until you add a new interface that is usable.

Quoted 1 line

I will modify sp_iop_read() to monitor IOP mailbox data.

Why is this a useful interface to have? If this is only for debugging, a tracepoint
may be more useful than a full character device.

Quoted 9 lines
Quoted 4 lines

Something looks wrong here, maybe reread the documentation for runtime power management to find a way of putting the device into low-power mode when it is unused.

When the poweroff command is executed, the run sp_iop_platform_driver_poweroff(void) function will enter the standby mode. The power off will be executed. In the system, IOP can continue to work when other modules in the system enter standby / power down modes to monitor whether the system wakes up through RTC.

Ok, in that case you can probably just remove the empty callback functions.

Arnd

#17 lore raw

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

Dear Arnd:

Quoted 30 lines

-----Original Message-----
From: Arnd Bergmann <[email protected]>
Sent: Monday, December 6, 2021 4:04 PM
To: Tony Huang 黃懷厚 <[email protected]>
Cc: Arnd Bergmann <[email protected]>; Tony Huang <[email protected]>; Derek Kiernan <[email protected]>; Dragan Cvetic <[email protected]>; gregkh <[email protected]>; Linux Kernel Mailing List <[email protected]>; Rob Herring <[email protected]>; DTML <[email protected]>; Wells Lu 呂芳騰 <[email protected]>
Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

Quoted 2 lines · Tony Huang 黃懷厚
Quoted 2 lines

Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 On Fri, Dec 3, 2021 at 4:48 AM Tony Huang

Quoted 5 lines · <[email protected]> wrote:
Quoted 5 lines
Quoted 5 lines
drivers/misc/iop/sunplus_iop.c new file +5 -0
+
+static const struct file_operations sp_iop_fops = {
+       .owner                  = THIS_MODULE,
+       .open                   = sp_iop_open,
+       .read                   = sp_iop_read,
Quoted 8 lines
Quoted 8 lines

drivers/misc/iop/sunplus_iop.c line 372

Quoted 3 lines
drivers/misc/iop/sunplus_iop.c new file +3 -0
+       .write                  = sp_iop_write,
+       .release                = sp_iop_release,
+};

This does nothing because all the callbacks are empty. You removed the inappropriate user space interfaces as I asked you to, but if there is no way for either kernel or user space to interact with the hardware, I don't see a point in merging the driver until you add a new

Quoted 8 lines

interface that is usable.

Quoted 3 lines
Quoted 1 line

I will modify sp_iop_read() to monitor IOP mailbox data.

Why is this a useful interface to have? If this is only for debugging, a tracepoint may be more useful than a full character device.

Yes, it is for debugging.

Quoted 15 lines
Quoted 10 lines
Quoted 4 lines

Something looks wrong here, maybe reread the documentation for runtime power management to find a way of putting the device into low-power mode when it is unused.

When the poweroff command is executed, the run sp_iop_platform_driver_poweroff(void)
function will enter the standby mode. The power off will be executed. In the system, IOP can continue to work when other modules in the system enter standby / power down modes to monitor whether the system

wakes up through RTC.

Ok, in that case you can probably just remove the empty callback functions.

Arnd