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

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

 Documentation/ABI/testing/sysfs-platform-soc@B     |  25 ++
 .../devicetree/bindings/misc/sunplus-iop.yaml      |  76 ++++
 MAINTAINERS                                        |   7 +
 drivers/misc/Kconfig                               |  12 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/sunplus_iop.c                         | 476 +++++++++++++++++++++
 6 files changed, 597 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-soc@B
 create mode 100644 Documentation/devicetree/bindings/misc/sunplus-iop.yaml
 create mode 100644 drivers/misc/sunplus_iop.c

-- 
2.7.4

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

[Tony Huang]

Add iop yaml file for Sunplus SP7021

Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
---
Changes in v6:
 -Modify incorrect name of gpio. Added wakeup-gpios pin for 8051.

 .../devicetree/bindings/misc/sunplus-iop.yaml      | 76 ++++++++++++++++++++++
 MAINTAINERS                                        |  5 ++
 2 files changed, 81 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..b37e697
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/sunplus-iop.yaml
@@ -0,0 +1,76 @@
+# 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.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - 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";
+        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>;
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index fb18ce7..6f336c9 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/sunplu-iop.yaml
+
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
 M:	Rich Felker <dalias@libc.org>
-- 
2.7.4

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

[Tony Huang]

IOP(8051) embedded inside SP7021 which is used as
Processor for I/O control, monitor RTC interrupt 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 <tonyhuang.sunplus@gmail.com>
---
Changes in v6:
 - Added sysfs read/write description.
 - Modify sysfs read function.
 - Addressed comments from kernel test robot.

 Documentation/ABI/testing/sysfs-platform-soc@B |  25 ++
 MAINTAINERS                                    |   2 +
 drivers/misc/Kconfig                           |  12 +
 drivers/misc/Makefile                          |   1 +
 drivers/misc/sunplus_iop.c                     | 476 +++++++++++++++++++++++++
 5 files changed, 516 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-soc@B
 create mode 100644 drivers/misc/sunplus_iop.c

diff --git a/Documentation/ABI/testing/sysfs-platform-soc@B b/Documentation/ABI/testing/sysfs-platform-soc@B
new file mode 100644
index 0000000..6272919
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-soc@B
@@ -0,0 +1,25 @@
+What:		/sys/devices/platform/soc@B/9c000400.iop/sp_iop_mailbox
+Date:		December 2021
+KernelVersion:	5.16
+Contact:	Tony Huang <tonyhuang.sunplus@gmail.com>
+Description:
+		Show 8051 mailbox0 data.
+
+What:		/sys/devices/platform/soc@B/9c000400.iop/sp_iop_mode
+Date:		December 2021
+KernelVersion:	5.16
+Contact:	Tony Huang <tonyhuang.sunplus@gmail.com>
+Description:
+		Read-Write.
+
+		Write this file.
+		Operation mode of IOP is switched to standby mode by writing
+		"1" to sysfs.
+		Operation mode of IOP is switched to normal mode by writing
+		"0" to sysfs.
+
+		Read this file.
+		Show operation mode of IOP. "0" is normal mode. "1" is standby
+		mode.
+
+
diff --git a/MAINTAINERS b/MAINTAINERS
index 6f336c9..cbc8dff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18245,7 +18245,9 @@ F:	drivers/net/ethernet/dlink/sundance.c
 SUNPLUS IOP DRIVER
 M:	Tony Huang <tonyhuang.sunplus@gmail.com>
 S:	Maintained
+F:	Documentation/ABI/testing/sysfs-platform-soc@B
 F:	Documentation/devicetree/bindings/misc/sunplu-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..45655ea 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -470,6 +470,18 @@ 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
+	  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.
+
+	  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..a16d9e6
--- /dev/null
+++ b/drivers/misc/sunplus_iop.c
@@ -0,0 +1,476 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * The IOP driver for Sunplus SP7021
+ *
+ * Copyright (C) 2021 Sunplus Technology Inc.
+ *
+ * All Rights Reserved.
+ */
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/firmware.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+
+enum IOP_Status_e {
+	IOP_SUCCESS,                /* successful */
+	IOP_ERR_IOP_BUSY,           /* IOP is busy */
+};
+
+struct regs_moon0 {
+	u32 stamp;         /* 00 */
+	u32 clken[10];     /* 01~10 */
+	u32 gclken[10];    /* 11~20 */
+	u32 reset[10];     /* 21~30 */
+	u32 sfg_cfg_mode;  /* 31 */
+};
+
+struct regs_iop {
+	u32 iop_control;/* 00 */
+	u32 iop_reg1;/* 01 */
+	u32 iop_bp;/* 02 */
+	u32 iop_regsel;/* 03 */
+	u32 iop_regout;/* 04 */
+	u32 iop_reg5;/* 05 */
+	u32 iop_resume_pcl;/* 06 */
+	u32 iop_resume_pch;/* 07 */
+	u32 iop_data0;/* 08 */
+	u32 iop_data1;/* 09 */
+	u32 iop_data2;/* 10 */
+	u32 iop_data3;/* 11 */
+	u32 iop_data4;/* 12 */
+	u32 iop_data5;/* 13 */
+	u32 iop_data6;/* 14 */
+	u32 iop_data7;/* 15 */
+	u32 iop_data8;/* 16 */
+	u32 iop_data9;/* 17 */
+	u32 iop_data10;/* 18 */
+	u32 iop_data11;/* 19 */
+	u32 iop_base_adr_l;/* 20 */
+	u32 iop_base_adr_h;/* 21 */
+	u32 memory_bridge_control;/* 22 */
+	u32 iop_regmap_adr_l;/* 23 */
+	u32 iop_regmap_adr_h;/* 24 */
+	u32 iop_direct_adr;/* 25*/
+	u32 reserved[6];/* 26~31 */
+};
+
+struct regs_iop_pmc {
+	u32 PMC_TIMER;/* 00 */
+	u32 PMC_CTRL;/* 01 */
+	u32 XTAL27M_PASSWORD_I;/* 02 */
+	u32 XTAL27M_PASSWORD_II;/* 03 */
+	u32 XTAL32K_PASSWORD_I;/* 04 */
+	u32 XTAL32K_PASSWORD_II;/* 05 */
+	u32 CLK27M_PASSWORD_I;/* 06 */
+	u32 CLK27M_PASSWORD_II;/* 07 */
+	u32 PMC_TIMER2;/* 08 */
+	u32 reserved[23];/* 9~31 */
+};
+
+#define NORMAL_CODE_MAX_SIZE 0X1000
+#define STANDBY_CODE_MAX_SIZE 0x4000
+struct sp_iop {
+	struct miscdevice dev;			// iop device
+	struct mutex write_lock;
+	void *iop_regs;
+	void *pmc_regs;
+	void *moon0_regs;
+	int irq;
+	int gpio_wakeup;
+	unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE];
+	unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE];
+	resource_size_t iop_mem_start;
+	resource_size_t iop_mem_size;
+	bool mode;
+};
+
+static void sp_iop_normal_mode(struct sp_iop *iop)
+{
+	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
+	struct regs_moon0 *p_moon0_reg = (struct regs_moon0 *)iop->moon0_regs;
+	void *iop_kernel_base;
+	unsigned int reg;
+
+	iop_kernel_base = ioremap(iop->iop_mem_start, NORMAL_CODE_MAX_SIZE);
+	memset(iop_kernel_base, 0, NORMAL_CODE_MAX_SIZE);
+	memcpy(iop_kernel_base, iop->iop_normal_code, NORMAL_CODE_MAX_SIZE);
+
+	writel(0x00100010, &p_moon0_reg->clken[0]);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg |= 0x01;
+	writel(reg, &p_iop_reg->iop_control);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg &= ~(0x8000);
+	writel(reg, &p_iop_reg->iop_control);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg |= 0x0200;//disable watchdog event reset IOP
+	writel(reg, &p_iop_reg->iop_control);
+
+	reg = (iop->iop_mem_start & 0xFFFF);
+	writel(reg, &p_iop_reg->iop_base_adr_l);
+	reg	= (iop->iop_mem_start >> 16);
+	writel(reg, &p_iop_reg->iop_base_adr_h);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg &= ~(0x01);
+	writel(reg, &p_iop_reg->iop_control);
+	iop->mode = 0;
+}
+
+static void sp_iop_standby_mode(struct sp_iop *iop)
+{
+	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
+	struct regs_moon0 *p_moon0_reg = (struct regs_moon0 *)iop->moon0_regs;
+	void *iop_kernel_base;
+	unsigned long reg;
+
+	iop_kernel_base = ioremap(iop->iop_mem_start, STANDBY_CODE_MAX_SIZE);
+	memset(iop_kernel_base, 0, STANDBY_CODE_MAX_SIZE);
+	memcpy(iop_kernel_base, iop->iop_standby_code, STANDBY_CODE_MAX_SIZE);
+
+	writel(0x00100010, &p_moon0_reg->clken[0]);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg |= 0x01;
+	writel(reg, &p_iop_reg->iop_control);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg &= ~(0x8000);
+	writel(reg, &p_iop_reg->iop_control);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg |= 0x0200;//disable watchdog event reset IOP
+	writel(reg, &p_iop_reg->iop_control);
+
+	reg = (iop->iop_mem_start & 0xFFFF);
+	writel(reg, &p_iop_reg->iop_base_adr_l);
+	reg = (iop->iop_mem_start >> 16);
+	writel(reg, &p_iop_reg->iop_base_adr_h);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg &= ~(0x01);
+	writel(reg, &p_iop_reg->iop_control);
+	iop->mode = 1;
+}
+
+#define IOP_READY	0x4
+#define RISC_READY	0x8
+#define WAKEUP_PIN	0xFE02
+#define S1	0x5331
+#define S3	0x5333
+static int sp_iop_s3mode(struct device *dev, struct sp_iop *iop)
+{
+	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
+	struct regs_moon0 *p_moon0_reg = (struct regs_moon0 *)iop->moon0_regs;
+	struct regs_iop_pmc *p_iop_pmc_reg = (struct regs_iop_pmc *)iop->pmc_regs;
+	unsigned int reg;
+	int ret, value;
+
+	writel(0x00100010, &p_moon0_reg->clken[0]);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg &= ~(0x8000);
+	writel(reg, &p_iop_reg->iop_control);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg |= 0x1;
+	writel(reg, &p_iop_reg->iop_control);
+
+	//PMC set
+	writel(0x00010001, &p_iop_pmc_reg->PMC_TIMER);
+	reg = readl(&p_iop_pmc_reg->PMC_CTRL);
+	reg |= 0x23;// disable system reset PMC, enalbe power down 27M, enable gating 27M
+	writel(reg, &p_iop_pmc_reg->PMC_CTRL);
+
+	writel(0x55aa00ff, &p_iop_pmc_reg->XTAL27M_PASSWORD_I);
+	writel(0x00ff55aa, &p_iop_pmc_reg->XTAL27M_PASSWORD_II);
+	writel(0xaa00ff55, &p_iop_pmc_reg->XTAL32K_PASSWORD_I);
+	writel(0xff55aa00, &p_iop_pmc_reg->XTAL32K_PASSWORD_II);
+	writel(0xaaff0055, &p_iop_pmc_reg->CLK27M_PASSWORD_I);
+	writel(0x5500aaff, &p_iop_pmc_reg->CLK27M_PASSWORD_II);
+	writel(0x01000100, &p_iop_pmc_reg->PMC_TIMER2);
+
+	//IOP Hardware IP reset
+	reg = readl(&p_moon0_reg->reset[0]);
+	reg |= 0x10;
+	writel(reg, (&p_moon0_reg->reset[0]));
+	reg &= ~(0x10);
+	writel(reg, (&p_moon0_reg->reset[0]));
+
+	writel(0x00ff0085, (iop->moon0_regs + 32 * 4 * 1 + 4 * 1));
+
+	reg = readl(iop->moon0_regs + 32 * 4 * 1 + 4 * 2);
+	reg |= 0x08000800;
+	writel(reg, (iop->moon0_regs + 32 * 4 * 1 + 4 * 2));
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg |= 0x0200;//disable watchdog event reset IOP
+	writel(reg, &p_iop_reg->iop_control);
+
+	reg = (iop->iop_mem_start & 0xFFFF);
+	writel(reg, &p_iop_reg->iop_base_adr_l);
+	reg = (iop->iop_mem_start >> 16);
+	writel(reg, &p_iop_reg->iop_base_adr_h);
+
+	reg = readl(&p_iop_reg->iop_control);
+	reg &= ~(0x01);
+	writel(reg, &p_iop_reg->iop_control);
+
+	writel(WAKEUP_PIN, &p_iop_reg->iop_data0);
+	writel(iop->gpio_wakeup, &p_iop_reg->iop_data1);
+
+	ret = readl_poll_timeout(&p_iop_reg->iop_data2, value,
+				 (value & IOP_READY) == IOP_READY, 1000, 10000);
+	if (ret) {
+		dev_err(dev, "timed out\n");
+		return ret;
+	}
+
+	writel(RISC_READY, &p_iop_reg->iop_data2);
+	writel(0x00, &p_iop_reg->iop_data5);
+	writel(0x60, &p_iop_reg->iop_data6);
+
+	ret = readl_poll_timeout(&p_iop_reg->iop_data7, value,
+				 value == 0xaaaa, 1000, 10000);
+	if (ret) {
+		dev_err(dev, "timed out\n");
+		return ret;
+	}
+
+	writel(0xdd, &p_iop_reg->iop_data1);//8051 bin file call Ultra low function.
+	mdelay(10);
+	return 0;
+}
+
+static int sp_iop_s1mode(struct device *dev, struct sp_iop *iop)
+{
+	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
+	int ret, value;
+
+	ret = readl_poll_timeout(&p_iop_reg->iop_data2, value,
+				 (value & IOP_READY) == IOP_READY, 1000, 10000);
+	if (ret) {
+		dev_err(dev, "timed out\n");
+		return ret;
+	}
+
+	writel(RISC_READY, &p_iop_reg->iop_data2);
+	writel(0x00, &p_iop_reg->iop_data5);
+	writel(0x60, &p_iop_reg->iop_data6);
+
+	ret = readl_poll_timeout(&p_iop_reg->iop_data7, value,
+				 value == 0xaaaa, 1000, 10000);
+	if (ret) {
+		dev_err(dev, "timed out\n");
+		return ret;
+	}
+
+	writel(0xee, &p_iop_reg->iop_data1);//8051 bin file call S1_mode function.
+	mdelay(10);
+	return 0;
+}
+
+static ssize_t sp_iop_mailbox_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct sp_iop *iop = dev_get_drvdata(dev);
+	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
+	unsigned int mailbox;
+
+	mailbox = readl(&p_iop_reg->iop_data0);
+	return sysfs_emit(buf, "mailbox = 0x%x\n", mailbox);
+}
+
+static ssize_t sp_iop_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct sp_iop *iop = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "bin code mode = 0x%x\n", iop->mode);
+}
+
+static ssize_t sp_iop_mode_store(struct device *dev, struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct sp_iop *iop = dev_get_drvdata(dev);
+
+	if (sysfs_streq(buf, "0"))
+		sp_iop_normal_mode(iop);
+	else if (sysfs_streq(buf, "1"))
+		sp_iop_standby_mode(iop);
+	return count;
+}
+
+static DEVICE_ATTR_RO(sp_iop_mailbox);
+static DEVICE_ATTR_RW(sp_iop_mode);
+
+static int  sp_iop_get_normal_code(struct device *dev, struct sp_iop *iop)
+{
+	const struct firmware *fw;
+	static const char file[] = "normal.bin";
+	unsigned int err, i;
+
+	err = request_firmware(&fw, file, dev);
+	if (err) {
+		dev_err(dev, "get bin file error\n");
+		return err;
+	}
+
+	for (i = 0; i < NORMAL_CODE_MAX_SIZE; i++) {
+		char temp;
+
+		temp = fw->data[i];
+		iop->iop_normal_code[i] = temp;
+	}
+	release_firmware(fw);
+	return err;
+}
+
+static int  sp_iop_get_standby_code(struct device *dev, struct sp_iop *iop)
+{
+	const struct firmware *fw;
+	static const char file[] = "standby.bin";
+	unsigned int err, i;
+
+	err = request_firmware(&fw, file, dev);
+	if (err) {
+		dev_err(dev, "get bin file error\n");
+		return err;
+	}
+
+	for (i = 0; i < STANDBY_CODE_MAX_SIZE; i++) {
+		char temp;
+
+		temp = fw->data[i];
+		iop->iop_standby_code[i] = temp;
+	}
+	release_firmware(fw);
+	return err;
+}
+
+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 IOP_SUCCESS;
+}
+
+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;
+	}
+	/* init */
+	mutex_init(&iop->write_lock);
+	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->iop_mem_start = mem_res.start;
+	iop->iop_mem_size = resource_size(&mem_res);
+
+	ret = sp_iop_get_normal_code(&pdev->dev, iop);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "get normal code err=%d\n", ret);
+		return ret;
+	}
+
+	ret = sp_iop_get_standby_code(&pdev->dev, iop);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "get standby code err=%d\n", ret);
+		return ret;
+	}
+
+	sp_iop_normal_mode(iop);
+	platform_set_drvdata(pdev, iop);
+	device_create_file(&pdev->dev, &dev_attr_sp_iop_mailbox);
+	device_create_file(&pdev->dev, &dev_attr_sp_iop_mode);
+	iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node, "iop-wakeup", 0);
+	return 0;
+
+fail_kmalloc:
+	return ret;
+}
+
+static void sp_iop_platform_driver_shutdown(struct platform_device *pdev)
+{
+	struct sp_iop *iop = platform_get_drvdata(pdev);
+	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
+	unsigned int value;
+
+	sp_iop_standby_mode(iop);
+	mdelay(10);
+	value = readl(&p_iop_reg->iop_data11);
+	if (value == S1)
+		sp_iop_s1mode(&pdev->dev, iop);
+	else
+		sp_iop_s3mode(&pdev->dev, iop);
+}
+
+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,
+	.shutdown	= sp_iop_platform_driver_shutdown,
+	.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 v6 2/2] misc: Add iop driver for Sunplus SP7021

[Greg KH]

On Thu, Dec 30, 2021 at 08:51:45PM +0800, Tony Huang wrote:
> IOP(8051) embedded inside SP7021 which is used as
> Processor for I/O control, monitor RTC interrupt 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 <tonyhuang.sunplus@gmail.com>
> ---
> Changes in v6:
>  - Added sysfs read/write description.
>  - Modify sysfs read function.
>  - Addressed comments from kernel test robot.
> 
>  Documentation/ABI/testing/sysfs-platform-soc@B |  25 ++
>  MAINTAINERS                                    |   2 +
>  drivers/misc/Kconfig                           |  12 +
>  drivers/misc/Makefile                          |   1 +
>  drivers/misc/sunplus_iop.c                     | 476 +++++++++++++++++++++++++
>  5 files changed, 516 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-soc@B
>  create mode 100644 drivers/misc/sunplus_iop.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-soc@B b/Documentation/ABI/testing/sysfs-platform-soc@B
> new file mode 100644
> index 0000000..6272919
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-soc@B
> @@ -0,0 +1,25 @@
> +What:		/sys/devices/platform/soc@B/9c000400.iop/sp_iop_mailbox
> +Date:		December 2021
> +KernelVersion:	5.16
> +Contact:	Tony Huang <tonyhuang.sunplus@gmail.com>
> +Description:
> +		Show 8051 mailbox0 data.

What format is the data in?



> +
> +What:		/sys/devices/platform/soc@B/9c000400.iop/sp_iop_mode
> +Date:		December 2021
> +KernelVersion:	5.16
> +Contact:	Tony Huang <tonyhuang.sunplus@gmail.com>
> +Description:
> +		Read-Write.
> +
> +		Write this file.
> +		Operation mode of IOP is switched to standby mode by writing
> +		"1" to sysfs.
> +		Operation mode of IOP is switched to normal mode by writing
> +		"0" to sysfs.
> +
> +		Read this file.
> +		Show operation mode of IOP. "0" is normal mode. "1" is standby
> +		mode.
> +
> +
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6f336c9..cbc8dff 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18245,7 +18245,9 @@ F:	drivers/net/ethernet/dlink/sundance.c
>  SUNPLUS IOP DRIVER
>  M:	Tony Huang <tonyhuang.sunplus@gmail.com>
>  S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-platform-soc@B
>  F:	Documentation/devicetree/bindings/misc/sunplu-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..45655ea 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -470,6 +470,18 @@ 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
> +	  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.

I do not understand this sentence, what do you mean by it?  Can you
provide a link to where the files that are required are?  Why not
include them in the linux-firmware project?

> +
> +	  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..a16d9e6
> --- /dev/null
> +++ b/drivers/misc/sunplus_iop.c
> @@ -0,0 +1,476 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * The IOP driver for Sunplus SP7021
> + *
> + * Copyright (C) 2021 Sunplus Technology Inc.
> + *
> + * All Rights Reserved.
> + */
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/firmware.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/miscdevice.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +
> +enum IOP_Status_e {
> +	IOP_SUCCESS,                /* successful */
> +	IOP_ERR_IOP_BUSY,           /* IOP is busy */
> +};
> +
> +struct regs_moon0 {
> +	u32 stamp;         /* 00 */
> +	u32 clken[10];     /* 01~10 */
> +	u32 gclken[10];    /* 11~20 */
> +	u32 reset[10];     /* 21~30 */
> +	u32 sfg_cfg_mode;  /* 31 */

What are these comments numbering?

> +};
> +
> +struct regs_iop {
> +	u32 iop_control;/* 00 */
> +	u32 iop_reg1;/* 01 */
> +	u32 iop_bp;/* 02 */
> +	u32 iop_regsel;/* 03 */
> +	u32 iop_regout;/* 04 */
> +	u32 iop_reg5;/* 05 */
> +	u32 iop_resume_pcl;/* 06 */
> +	u32 iop_resume_pch;/* 07 */
> +	u32 iop_data0;/* 08 */
> +	u32 iop_data1;/* 09 */
> +	u32 iop_data2;/* 10 */
> +	u32 iop_data3;/* 11 */
> +	u32 iop_data4;/* 12 */
> +	u32 iop_data5;/* 13 */
> +	u32 iop_data6;/* 14 */
> +	u32 iop_data7;/* 15 */
> +	u32 iop_data8;/* 16 */
> +	u32 iop_data9;/* 17 */
> +	u32 iop_data10;/* 18 */
> +	u32 iop_data11;/* 19 */
> +	u32 iop_base_adr_l;/* 20 */
> +	u32 iop_base_adr_h;/* 21 */
> +	u32 memory_bridge_control;/* 22 */
> +	u32 iop_regmap_adr_l;/* 23 */
> +	u32 iop_regmap_adr_h;/* 24 */
> +	u32 iop_direct_adr;/* 25*/
> +	u32 reserved[6];/* 26~31 */

Same here, what are these numbers?

And why are they not lined up like the previous structure?


> +};
> +
> +struct regs_iop_pmc {
> +	u32 PMC_TIMER;/* 00 */
> +	u32 PMC_CTRL;/* 01 */
> +	u32 XTAL27M_PASSWORD_I;/* 02 */
> +	u32 XTAL27M_PASSWORD_II;/* 03 */
> +	u32 XTAL32K_PASSWORD_I;/* 04 */
> +	u32 XTAL32K_PASSWORD_II;/* 05 */
> +	u32 CLK27M_PASSWORD_I;/* 06 */
> +	u32 CLK27M_PASSWORD_II;/* 07 */
> +	u32 PMC_TIMER2;/* 08 */
> +	u32 reserved[23];/* 9~31 */

Same comment question here.

> +};
> +
> +#define NORMAL_CODE_MAX_SIZE 0X1000

"0x"?

> +#define STANDBY_CODE_MAX_SIZE 0x4000

What are these for?

> +struct sp_iop {
> +	struct miscdevice dev;			// iop device

Why the comment?

> +	struct mutex write_lock;
> +	void *iop_regs;
> +	void *pmc_regs;
> +	void *moon0_regs;

Why void pointers?  You created structures above, use them!

This is not Windows programming :)


> +	int irq;
> +	int gpio_wakeup;
> +	unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE];
> +	unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE];
> +	resource_size_t iop_mem_start;
> +	resource_size_t iop_mem_size;
> +	bool mode;

How can a "mode" be boolean?  Perhaps a better name?

> +};
> +
> +static void sp_iop_normal_mode(struct sp_iop *iop)
> +{
> +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;

See, don't use a void pointer, use the real structure please.

> +	struct regs_moon0 *p_moon0_reg = (struct regs_moon0 *)iop->moon0_regs;
> +	void *iop_kernel_base;

Why void?  Isn't this a structure too?

> +	unsigned int reg;
> +
> +	iop_kernel_base = ioremap(iop->iop_mem_start, NORMAL_CODE_MAX_SIZE);
> +	memset(iop_kernel_base, 0, NORMAL_CODE_MAX_SIZE);
> +	memcpy(iop_kernel_base, iop->iop_normal_code, NORMAL_CODE_MAX_SIZE);
> +
> +	writel(0x00100010, &p_moon0_reg->clken[0]);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg |= 0x01;
> +	writel(reg, &p_iop_reg->iop_control);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg &= ~(0x8000);
> +	writel(reg, &p_iop_reg->iop_control);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg |= 0x0200;//disable watchdog event reset IOP
> +	writel(reg, &p_iop_reg->iop_control);
> +
> +	reg = (iop->iop_mem_start & 0xFFFF);
> +	writel(reg, &p_iop_reg->iop_base_adr_l);
> +	reg	= (iop->iop_mem_start >> 16);
> +	writel(reg, &p_iop_reg->iop_base_adr_h);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg &= ~(0x01);
> +	writel(reg, &p_iop_reg->iop_control);
> +	iop->mode = 0;
> +}
> +
> +static void sp_iop_standby_mode(struct sp_iop *iop)
> +{
> +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> +	struct regs_moon0 *p_moon0_reg = (struct regs_moon0 *)iop->moon0_regs;
> +	void *iop_kernel_base;
> +	unsigned long reg;
> +
> +	iop_kernel_base = ioremap(iop->iop_mem_start, STANDBY_CODE_MAX_SIZE);
> +	memset(iop_kernel_base, 0, STANDBY_CODE_MAX_SIZE);
> +	memcpy(iop_kernel_base, iop->iop_standby_code, STANDBY_CODE_MAX_SIZE);
> +
> +	writel(0x00100010, &p_moon0_reg->clken[0]);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg |= 0x01;
> +	writel(reg, &p_iop_reg->iop_control);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg &= ~(0x8000);
> +	writel(reg, &p_iop_reg->iop_control);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg |= 0x0200;//disable watchdog event reset IOP
> +	writel(reg, &p_iop_reg->iop_control);
> +
> +	reg = (iop->iop_mem_start & 0xFFFF);
> +	writel(reg, &p_iop_reg->iop_base_adr_l);
> +	reg = (iop->iop_mem_start >> 16);
> +	writel(reg, &p_iop_reg->iop_base_adr_h);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg &= ~(0x01);
> +	writel(reg, &p_iop_reg->iop_control);
> +	iop->mode = 1;
> +}
> +
> +#define IOP_READY	0x4
> +#define RISC_READY	0x8
> +#define WAKEUP_PIN	0xFE02
> +#define S1	0x5331
> +#define S3	0x5333


What are these values for?

> +static int sp_iop_s3mode(struct device *dev, struct sp_iop *iop)
> +{
> +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> +	struct regs_moon0 *p_moon0_reg = (struct regs_moon0 *)iop->moon0_regs;
> +	struct regs_iop_pmc *p_iop_pmc_reg = (struct regs_iop_pmc *)iop->pmc_regs;
> +	unsigned int reg;
> +	int ret, value;
> +
> +	writel(0x00100010, &p_moon0_reg->clken[0]);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg &= ~(0x8000);
> +	writel(reg, &p_iop_reg->iop_control);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg |= 0x1;
> +	writel(reg, &p_iop_reg->iop_control);
> +
> +	//PMC set
> +	writel(0x00010001, &p_iop_pmc_reg->PMC_TIMER);
> +	reg = readl(&p_iop_pmc_reg->PMC_CTRL);
> +	reg |= 0x23;// disable system reset PMC, enalbe power down 27M, enable gating 27M

What is "27M"?

> +	writel(reg, &p_iop_pmc_reg->PMC_CTRL);
> +
> +	writel(0x55aa00ff, &p_iop_pmc_reg->XTAL27M_PASSWORD_I);
> +	writel(0x00ff55aa, &p_iop_pmc_reg->XTAL27M_PASSWORD_II);
> +	writel(0xaa00ff55, &p_iop_pmc_reg->XTAL32K_PASSWORD_I);
> +	writel(0xff55aa00, &p_iop_pmc_reg->XTAL32K_PASSWORD_II);
> +	writel(0xaaff0055, &p_iop_pmc_reg->CLK27M_PASSWORD_I);
> +	writel(0x5500aaff, &p_iop_pmc_reg->CLK27M_PASSWORD_II);
> +	writel(0x01000100, &p_iop_pmc_reg->PMC_TIMER2);
> +
> +	//IOP Hardware IP reset

Always put a ' ' after '//'


> +	reg = readl(&p_moon0_reg->reset[0]);
> +	reg |= 0x10;
> +	writel(reg, (&p_moon0_reg->reset[0]));
> +	reg &= ~(0x10);
> +	writel(reg, (&p_moon0_reg->reset[0]));
> +
> +	writel(0x00ff0085, (iop->moon0_regs + 32 * 4 * 1 + 4 * 1));
> +
> +	reg = readl(iop->moon0_regs + 32 * 4 * 1 + 4 * 2);
> +	reg |= 0x08000800;
> +	writel(reg, (iop->moon0_regs + 32 * 4 * 1 + 4 * 2));
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg |= 0x0200;//disable watchdog event reset IOP
> +	writel(reg, &p_iop_reg->iop_control);
> +
> +	reg = (iop->iop_mem_start & 0xFFFF);
> +	writel(reg, &p_iop_reg->iop_base_adr_l);
> +	reg = (iop->iop_mem_start >> 16);
> +	writel(reg, &p_iop_reg->iop_base_adr_h);
> +
> +	reg = readl(&p_iop_reg->iop_control);
> +	reg &= ~(0x01);
> +	writel(reg, &p_iop_reg->iop_control);
> +
> +	writel(WAKEUP_PIN, &p_iop_reg->iop_data0);
> +	writel(iop->gpio_wakeup, &p_iop_reg->iop_data1);
> +
> +	ret = readl_poll_timeout(&p_iop_reg->iop_data2, value,
> +				 (value & IOP_READY) == IOP_READY, 1000, 10000);
> +	if (ret) {
> +		dev_err(dev, "timed out\n");
> +		return ret;
> +	}
> +
> +	writel(RISC_READY, &p_iop_reg->iop_data2);
> +	writel(0x00, &p_iop_reg->iop_data5);
> +	writel(0x60, &p_iop_reg->iop_data6);
> +
> +	ret = readl_poll_timeout(&p_iop_reg->iop_data7, value,
> +				 value == 0xaaaa, 1000, 10000);
> +	if (ret) {
> +		dev_err(dev, "timed out\n");
> +		return ret;
> +	}
> +
> +	writel(0xdd, &p_iop_reg->iop_data1);//8051 bin file call Ultra low function.
> +	mdelay(10);

Where did 10 come from?  How do you know that is correct?

Same comment for your other delay calls in the driver.

> +	return 0;
> +}
> +
> +static int sp_iop_s1mode(struct device *dev, struct sp_iop *iop)
> +{
> +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> +	int ret, value;
> +
> +	ret = readl_poll_timeout(&p_iop_reg->iop_data2, value,
> +				 (value & IOP_READY) == IOP_READY, 1000, 10000);
> +	if (ret) {
> +		dev_err(dev, "timed out\n");
> +		return ret;
> +	}
> +
> +	writel(RISC_READY, &p_iop_reg->iop_data2);
> +	writel(0x00, &p_iop_reg->iop_data5);
> +	writel(0x60, &p_iop_reg->iop_data6);
> +
> +	ret = readl_poll_timeout(&p_iop_reg->iop_data7, value,
> +				 value == 0xaaaa, 1000, 10000);
> +	if (ret) {
> +		dev_err(dev, "timed out\n");
> +		return ret;
> +	}
> +
> +	writel(0xee, &p_iop_reg->iop_data1);//8051 bin file call S1_mode function.
> +	mdelay(10);
> +	return 0;
> +}
> +
> +static ssize_t sp_iop_mailbox_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct sp_iop *iop = dev_get_drvdata(dev);
> +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> +	unsigned int mailbox;
> +
> +	mailbox = readl(&p_iop_reg->iop_data0);
> +	return sysfs_emit(buf, "mailbox = 0x%x\n", mailbox);
> +}
> +
> +static ssize_t sp_iop_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct sp_iop *iop = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "bin code mode = 0x%x\n", iop->mode);

That is not a valid sysfs file output.  Again "ONE VALUE PER FILE", you
should never have to parse the output like this.

> +}
> +
> +static ssize_t sp_iop_mode_store(struct device *dev, struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct sp_iop *iop = dev_get_drvdata(dev);
> +
> +	if (sysfs_streq(buf, "0"))
> +		sp_iop_normal_mode(iop);
> +	else if (sysfs_streq(buf, "1"))
> +		sp_iop_standby_mode(iop);
> +	return count;

So no matter what you write here, it will not return an error?

That is not correct.

> +}
> +
> +static DEVICE_ATTR_RO(sp_iop_mailbox);
> +static DEVICE_ATTR_RW(sp_iop_mode);
> +
> +static int  sp_iop_get_normal_code(struct device *dev, struct sp_iop *iop)
> +{
> +	const struct firmware *fw;
> +	static const char file[] = "normal.bin";
> +	unsigned int err, i;
> +
> +	err = request_firmware(&fw, file, dev);
> +	if (err) {
> +		dev_err(dev, "get bin file error\n");
> +		return err;
> +	}
> +
> +	for (i = 0; i < NORMAL_CODE_MAX_SIZE; i++) {
> +		char temp;
> +
> +		temp = fw->data[i];
> +		iop->iop_normal_code[i] = temp;
> +	}
> +	release_firmware(fw);
> +	return err;
> +}
> +
> +static int  sp_iop_get_standby_code(struct device *dev, struct sp_iop *iop)
> +{
> +	const struct firmware *fw;
> +	static const char file[] = "standby.bin";
> +	unsigned int err, i;
> +
> +	err = request_firmware(&fw, file, dev);
> +	if (err) {
> +		dev_err(dev, "get bin file error\n");
> +		return err;
> +	}
> +
> +	for (i = 0; i < STANDBY_CODE_MAX_SIZE; i++) {
> +		char temp;
> +
> +		temp = fw->data[i];
> +		iop->iop_standby_code[i] = temp;
> +	}
> +	release_firmware(fw);
> +	return err;
> +}
> +
> +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 IOP_SUCCESS;
> +}
> +
> +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;
> +	}
> +	/* init */
> +	mutex_init(&iop->write_lock);
> +	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->iop_mem_start = mem_res.start;
> +	iop->iop_mem_size = resource_size(&mem_res);
> +
> +	ret = sp_iop_get_normal_code(&pdev->dev, iop);
> +	if (ret != 0) {
> +		dev_err(&pdev->dev, "get normal code err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = sp_iop_get_standby_code(&pdev->dev, iop);
> +	if (ret != 0) {
> +		dev_err(&pdev->dev, "get standby code err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	sp_iop_normal_mode(iop);
> +	platform_set_drvdata(pdev, iop);
> +	device_create_file(&pdev->dev, &dev_attr_sp_iop_mailbox);
> +	device_create_file(&pdev->dev, &dev_attr_sp_iop_mode);

You just raced with userspace and lost.  Set the default groups pointer
of the misc device to your attribute group and then they will be
automatically created for you.




> +	iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node, "iop-wakeup", 0);
> +	return 0;
> +
> +fail_kmalloc:
> +	return ret;
> +}
> +
> +static void sp_iop_platform_driver_shutdown(struct platform_device *pdev)
> +{
> +	struct sp_iop *iop = platform_get_drvdata(pdev);
> +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> +	unsigned int value;
> +
> +	sp_iop_standby_mode(iop);
> +	mdelay(10);

Why sleep on shutdown?

thanks,

greg k-h

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

[Tony Huang 黃懷厚]

Dear Greg:

> > IOP(8051) embedded inside SP7021 which is used as Processor for I/O
> > control, monitor RTC interrupt 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 <tonyhuang.sunplus@gmail.com>
> > ---
> > Changes in v6:
> >  - Added sysfs read/write description.
> >  - Modify sysfs read function.
> >  - Addressed comments from kernel test robot.
> >
> >  Documentation/ABI/testing/sysfs-platform-soc@B |  25 ++
> >  MAINTAINERS                                    |   2 +
> >  drivers/misc/Kconfig                           |  12 +
> >  drivers/misc/Makefile                          |   1 +
> >  drivers/misc/sunplus_iop.c                     | 476
> +++++++++++++++++++++++++
> >  5 files changed, 516 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-platform-soc@B
> >  create mode 100644 drivers/misc/sunplus_iop.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-platform-soc@B
> > b/Documentation/ABI/testing/sysfs-platform-soc@B
> > new file mode 100644
> > index 0000000..6272919
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-platform-soc@B
> > @@ -0,0 +1,25 @@
> > +What:		/sys/devices/platform/soc@B/9c000400.iop/sp_iop_mailbox
> > +Date:		December 2021
> > +KernelVersion:	5.16
> > +Contact:	Tony Huang <tonyhuang.sunplus@gmail.com>
> > +Description:
> > +		Show 8051 mailbox0 data.
> 
> What format is the data in?
> 

Unsigned short

> 
> 
> > +
> > +What:		/sys/devices/platform/soc@B/9c000400.iop/sp_iop_mode
> > +Date:		December 2021
> > +KernelVersion:	5.16
> > +Contact:	Tony Huang <tonyhuang.sunplus@gmail.com>
> > +Description:
> > +		Read-Write.
> > +
> > +		Write this file.
> > +		Operation mode of IOP is switched to standby mode by writing
> > +		"1" to sysfs.
> > +		Operation mode of IOP is switched to normal mode by writing
> > +		"0" to sysfs.
> > +
> > +		Read this file.
> > +		Show operation mode of IOP. "0" is normal mode. "1" is standby
> > +		mode.
> > +
> > +
> > diff --git a/MAINTAINERS b/MAINTAINERS index 6f336c9..cbc8dff 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18245,7 +18245,9 @@ F:	drivers/net/ethernet/dlink/sundance.c
> >  SUNPLUS IOP DRIVER
> >  M:	Tony Huang <tonyhuang.sunplus@gmail.com>
> >  S:	Maintained
> > +F:	Documentation/ABI/testing/sysfs-platform-soc@B
> >  F:	Documentation/devicetree/bindings/misc/sunplu-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..45655ea 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -470,6 +470,18 @@ 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
> > +	  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.
> 
> I do not understand this sentence, what do you mean by it?  Can you provide a
> link to where the files that are required are?  Why not include them in the
> linux-firmware project?
> 

1.We will provide users with 8051 normal and standby 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			
2.Users can follow the operation steps to generate normal.bin and standby.bin.			
  Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051			
  26.5 How To Create 8051 bin file			

> > +
> > +	  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..a16d9e6
> > --- /dev/null
> > +++ b/drivers/misc/sunplus_iop.c
> > @@ -0,0 +1,476 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * The IOP driver for Sunplus SP7021
> > + *
> > + * Copyright (C) 2021 Sunplus Technology Inc.
> > + *
> > + * All Rights Reserved.
> > + */
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/firmware.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/module.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_gpio.h>
> > +
> > +enum IOP_Status_e {
> > +	IOP_SUCCESS,                /* successful */
> > +	IOP_ERR_IOP_BUSY,           /* IOP is busy */
> > +};
> > +
> > +struct regs_moon0 {
> > +	u32 stamp;         /* 00 */
> > +	u32 clken[10];     /* 01~10 */
> > +	u32 gclken[10];    /* 11~20 */
> > +	u32 reset[10];     /* 21~30 */
> > +	u32 sfg_cfg_mode;  /* 31 */
> 
> What are these comments numbering?
> 

regs_moon0 is Group 0 moon register.					
The Group0 moon register range is 0x9c00000~0x9c00007F					
/*00*/: 0x9c000000~0x9c000003					
/*01~10*/:0x9c000004~0x9c00002b					
/*11~20*/:0x9c00002c~0x9c000053					
/*21~30*/:0x9c000054~0x9c00007b					
/*31*/:0x9c00007c~0x9c00007f					

> > +};
> > +
> > +struct regs_iop {
> > +	u32 iop_control;/* 00 */
> > +	u32 iop_reg1;/* 01 */
> > +	u32 iop_bp;/* 02 */
> > +	u32 iop_regsel;/* 03 */
> > +	u32 iop_regout;/* 04 */
> > +	u32 iop_reg5;/* 05 */
> > +	u32 iop_resume_pcl;/* 06 */
> > +	u32 iop_resume_pch;/* 07 */
> > +	u32 iop_data0;/* 08 */
> > +	u32 iop_data1;/* 09 */
> > +	u32 iop_data2;/* 10 */
> > +	u32 iop_data3;/* 11 */
> > +	u32 iop_data4;/* 12 */
> > +	u32 iop_data5;/* 13 */
> > +	u32 iop_data6;/* 14 */
> > +	u32 iop_data7;/* 15 */
> > +	u32 iop_data8;/* 16 */
> > +	u32 iop_data9;/* 17 */
> > +	u32 iop_data10;/* 18 */
> > +	u32 iop_data11;/* 19 */
> > +	u32 iop_base_adr_l;/* 20 */
> > +	u32 iop_base_adr_h;/* 21 */
> > +	u32 memory_bridge_control;/* 22 */
> > +	u32 iop_regmap_adr_l;/* 23 */
> > +	u32 iop_regmap_adr_h;/* 24 */
> > +	u32 iop_direct_adr;/* 25*/
> > +	u32 reserved[6];/* 26~31 */
> 
> Same here, what are these numbers?
> 
> And why are they not lined up like the previous structure?
> 

Sorry, I don't understand what you mean. Isn't this a struct?					

> 
> > +};
> > +
> > +struct regs_iop_pmc {
> > +	u32 PMC_TIMER;/* 00 */
> > +	u32 PMC_CTRL;/* 01 */
> > +	u32 XTAL27M_PASSWORD_I;/* 02 */
> > +	u32 XTAL27M_PASSWORD_II;/* 03 */
> > +	u32 XTAL32K_PASSWORD_I;/* 04 */
> > +	u32 XTAL32K_PASSWORD_II;/* 05 */
> > +	u32 CLK27M_PASSWORD_I;/* 06 */
> > +	u32 CLK27M_PASSWORD_II;/* 07 */
> > +	u32 PMC_TIMER2;/* 08 */
> > +	u32 reserved[23];/* 9~31 */
> 
> Same comment question here.
> 
> > +};
> > +
> > +#define NORMAL_CODE_MAX_SIZE 0X1000
> 
> "0x"?
> 

This is the maximum size of normal.bin that can be received.					

> > +#define STANDBY_CODE_MAX_SIZE 0x4000
> 
> What are these for?
> 

This is the maximum size of standby.bin that can be received.					

> > +struct sp_iop {
> > +	struct miscdevice dev;			// iop device
> 
> Why the comment?
> 

I will remove it.

> > +	struct mutex write_lock;
> > +	void *iop_regs;
> > +	void *pmc_regs;
> > +	void *moon0_regs;
> 
> Why void pointers?  You created structures above, use them!
> 

Because I received "Reported-by: kernel test robot <lkp@intel.com>", warmming message. As follows:										
sparse warnings: (new ones prefixed by >>)										
   drivers/misc/sunplus_iop.c:94:39: sparse: sparse: cast removes address space '__iomem' of expression										
   drivers/misc/sunplus_iop.c:95:43: sparse: sparse: cast removes address space '__iomem' of expression										
>> drivers/misc/sunplus_iop.c:100:16: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void *p @@     got void [noderef] __iomem *[assigned] iop_kernel_base @@										
   drivers/misc/sunplus_iop.c:100:16: sparse:     expected void *p										
   drivers/misc/sunplus_iop.c:100:16: sparse:     got void [noderef] __iomem *[assigned] iop_kernel_base										

> This is not Windows programming :)
> 

I will modify it.

> 
> > +	int irq;
> > +	int gpio_wakeup;
> > +	unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE];
> > +	unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE];
> > +	resource_size_t iop_mem_start;
> > +	resource_size_t iop_mem_size;
> > +	bool mode;
> 
> How can a "mode" be boolean?  Perhaps a better name?
> 

Ok. I will modify it.

> > +};
> > +
> > +static void sp_iop_normal_mode(struct sp_iop *iop) {
> > +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> 
> See, don't use a void pointer, use the real structure please.
> 

I will modify it.

> > +	struct regs_moon0 *p_moon0_reg = (struct regs_moon0
> *)iop->moon0_regs;
> > +	void *iop_kernel_base;
> 
> Why void?  Isn't this a structure too?
> 

I will modify it.

> > +	unsigned int reg;
> > +
> > +	iop_kernel_base = ioremap(iop->iop_mem_start,
> NORMAL_CODE_MAX_SIZE);
> > +	memset(iop_kernel_base, 0, NORMAL_CODE_MAX_SIZE);
> > +	memcpy(iop_kernel_base, iop->iop_normal_code,
> NORMAL_CODE_MAX_SIZE);
> > +
> > +	writel(0x00100010, &p_moon0_reg->clken[0]);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg |= 0x01;
> > +	writel(reg, &p_iop_reg->iop_control);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg &= ~(0x8000);
> > +	writel(reg, &p_iop_reg->iop_control);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg |= 0x0200;//disable watchdog event reset IOP
> > +	writel(reg, &p_iop_reg->iop_control);
> > +
> > +	reg = (iop->iop_mem_start & 0xFFFF);
> > +	writel(reg, &p_iop_reg->iop_base_adr_l);
> > +	reg	= (iop->iop_mem_start >> 16);
> > +	writel(reg, &p_iop_reg->iop_base_adr_h);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg &= ~(0x01);
> > +	writel(reg, &p_iop_reg->iop_control);
> > +	iop->mode = 0;
> > +}
> > +
> > +static void sp_iop_standby_mode(struct sp_iop *iop) {
> > +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> > +	struct regs_moon0 *p_moon0_reg = (struct regs_moon0
> *)iop->moon0_regs;
> > +	void *iop_kernel_base;
> > +	unsigned long reg;
> > +
> > +	iop_kernel_base = ioremap(iop->iop_mem_start,
> STANDBY_CODE_MAX_SIZE);
> > +	memset(iop_kernel_base, 0, STANDBY_CODE_MAX_SIZE);
> > +	memcpy(iop_kernel_base, iop->iop_standby_code,
> > +STANDBY_CODE_MAX_SIZE);
> > +
> > +	writel(0x00100010, &p_moon0_reg->clken[0]);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg |= 0x01;
> > +	writel(reg, &p_iop_reg->iop_control);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg &= ~(0x8000);
> > +	writel(reg, &p_iop_reg->iop_control);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg |= 0x0200;//disable watchdog event reset IOP
> > +	writel(reg, &p_iop_reg->iop_control);
> > +
> > +	reg = (iop->iop_mem_start & 0xFFFF);
> > +	writel(reg, &p_iop_reg->iop_base_adr_l);
> > +	reg = (iop->iop_mem_start >> 16);
> > +	writel(reg, &p_iop_reg->iop_base_adr_h);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg &= ~(0x01);
> > +	writel(reg, &p_iop_reg->iop_control);
> > +	iop->mode = 1;
> > +}
> > +
> > +#define IOP_READY	0x4
> > +#define RISC_READY	0x8
> > +#define WAKEUP_PIN	0xFE02
> > +#define S1	0x5331
> > +#define S3	0x5333
> 
> 
> What are these values for?
> 

These values ​​will be written in the mailbox register. System linux kernel and 8051 will read the mailbox register.
IOP_READY=0x4, RISC_READY=0x8: 8051 informs linux kernel.8051 has been switched to stanby.bin code.
WAKEUP_PIN=0xFE02: System linux kernel tells 8051 which gpio pin to wake up through.　
S1=0x5331, S3=0x5333: System linux kernel tells 8051 whether to execute S1 mode or S3 mode.

> > +static int sp_iop_s3mode(struct device *dev, struct sp_iop *iop) {
> > +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> > +	struct regs_moon0 *p_moon0_reg = (struct regs_moon0
> *)iop->moon0_regs;
> > +	struct regs_iop_pmc *p_iop_pmc_reg = (struct regs_iop_pmc
> *)iop->pmc_regs;
> > +	unsigned int reg;
> > +	int ret, value;
> > +
> > +	writel(0x00100010, &p_moon0_reg->clken[0]);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg &= ~(0x8000);
> > +	writel(reg, &p_iop_reg->iop_control);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg |= 0x1;
> > +	writel(reg, &p_iop_reg->iop_control);
> > +
> > +	//PMC set
> > +	writel(0x00010001, &p_iop_pmc_reg->PMC_TIMER);
> > +	reg = readl(&p_iop_pmc_reg->PMC_CTRL);
> > +	reg |= 0x23;// disable system reset PMC, enalbe power down 27M,
> > +enable gating 27M
> 
> What is "27M"?
> 

clock 27Mhz

> > +	writel(reg, &p_iop_pmc_reg->PMC_CTRL);
> > +
> > +	writel(0x55aa00ff, &p_iop_pmc_reg->XTAL27M_PASSWORD_I);
> > +	writel(0x00ff55aa, &p_iop_pmc_reg->XTAL27M_PASSWORD_II);
> > +	writel(0xaa00ff55, &p_iop_pmc_reg->XTAL32K_PASSWORD_I);
> > +	writel(0xff55aa00, &p_iop_pmc_reg->XTAL32K_PASSWORD_II);
> > +	writel(0xaaff0055, &p_iop_pmc_reg->CLK27M_PASSWORD_I);
> > +	writel(0x5500aaff, &p_iop_pmc_reg->CLK27M_PASSWORD_II);
> > +	writel(0x01000100, &p_iop_pmc_reg->PMC_TIMER2);
> > +
> > +	//IOP Hardware IP reset
> 
> Always put a ' ' after '//'
> 

OK, I will modify it.

> 
> > +	reg = readl(&p_moon0_reg->reset[0]);
> > +	reg |= 0x10;
> > +	writel(reg, (&p_moon0_reg->reset[0]));
> > +	reg &= ~(0x10);
> > +	writel(reg, (&p_moon0_reg->reset[0]));
> > +
> > +	writel(0x00ff0085, (iop->moon0_regs + 32 * 4 * 1 + 4 * 1));
> > +
> > +	reg = readl(iop->moon0_regs + 32 * 4 * 1 + 4 * 2);
> > +	reg |= 0x08000800;
> > +	writel(reg, (iop->moon0_regs + 32 * 4 * 1 + 4 * 2));
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg |= 0x0200;//disable watchdog event reset IOP
> > +	writel(reg, &p_iop_reg->iop_control);
> > +
> > +	reg = (iop->iop_mem_start & 0xFFFF);
> > +	writel(reg, &p_iop_reg->iop_base_adr_l);
> > +	reg = (iop->iop_mem_start >> 16);
> > +	writel(reg, &p_iop_reg->iop_base_adr_h);
> > +
> > +	reg = readl(&p_iop_reg->iop_control);
> > +	reg &= ~(0x01);
> > +	writel(reg, &p_iop_reg->iop_control);
> > +
> > +	writel(WAKEUP_PIN, &p_iop_reg->iop_data0);
> > +	writel(iop->gpio_wakeup, &p_iop_reg->iop_data1);
> > +
> > +	ret = readl_poll_timeout(&p_iop_reg->iop_data2, value,
> > +				 (value & IOP_READY) == IOP_READY, 1000, 10000);
> > +	if (ret) {
> > +		dev_err(dev, "timed out\n");
> > +		return ret;
> > +	}
> > +
> > +	writel(RISC_READY, &p_iop_reg->iop_data2);
> > +	writel(0x00, &p_iop_reg->iop_data5);
> > +	writel(0x60, &p_iop_reg->iop_data6);
> > +
> > +	ret = readl_poll_timeout(&p_iop_reg->iop_data7, value,
> > +				 value == 0xaaaa, 1000, 10000);
> > +	if (ret) {
> > +		dev_err(dev, "timed out\n");
> > +		return ret;
> > +	}
> > +
> > +	writel(0xdd, &p_iop_reg->iop_data1);//8051 bin file call Ultra low
> function.
> > +	mdelay(10);
> 
> Where did 10 come from?  How do you know that is correct?
> 

I need time to move stanby.bin code 16K data from SDRAM to 8051's icache.		
10msec should be enough		

> Same comment for your other delay calls in the driver.
> 
> > +	return 0;
> > +}
> > +
> > +static int sp_iop_s1mode(struct device *dev, struct sp_iop *iop) {
> > +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> > +	int ret, value;
> > +
> > +	ret = readl_poll_timeout(&p_iop_reg->iop_data2, value,
> > +				 (value & IOP_READY) == IOP_READY, 1000, 10000);
> > +	if (ret) {
> > +		dev_err(dev, "timed out\n");
> > +		return ret;
> > +	}
> > +
> > +	writel(RISC_READY, &p_iop_reg->iop_data2);
> > +	writel(0x00, &p_iop_reg->iop_data5);
> > +	writel(0x60, &p_iop_reg->iop_data6);
> > +
> > +	ret = readl_poll_timeout(&p_iop_reg->iop_data7, value,
> > +				 value == 0xaaaa, 1000, 10000);
> > +	if (ret) {
> > +		dev_err(dev, "timed out\n");
> > +		return ret;
> > +	}
> > +
> > +	writel(0xee, &p_iop_reg->iop_data1);//8051 bin file call S1_mode
> function.
> > +	mdelay(10);
> > +	return 0;
> > +}
> > +
> > +static ssize_t sp_iop_mailbox_show(struct device *dev, struct
> > +device_attribute *attr, char *buf) {
> > +	struct sp_iop *iop = dev_get_drvdata(dev);
> > +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> > +	unsigned int mailbox;
> > +
> > +	mailbox = readl(&p_iop_reg->iop_data0);
> > +	return sysfs_emit(buf, "mailbox = 0x%x\n", mailbox); }
> > +
> > +static ssize_t sp_iop_mode_show(struct device *dev, struct
> > +device_attribute *attr, char *buf) {
> > +	struct sp_iop *iop = dev_get_drvdata(dev);
> > +
> > +	return sysfs_emit(buf, "bin code mode = 0x%x\n", iop->mode);
> 
> That is not a valid sysfs file output.  Again "ONE VALUE PER FILE", you should
> never have to parse the output like this.
> 

I will change sysfs_emit(buf, "%x\n", iop->mode);

> > +}
> > +
> > +static ssize_t sp_iop_mode_store(struct device *dev, struct device_attribute
> *attr,
> > +				 const char *buf, size_t count)
> > +{
> > +	struct sp_iop *iop = dev_get_drvdata(dev);
> > +
> > +	if (sysfs_streq(buf, "0"))
> > +		sp_iop_normal_mode(iop);
> > +	else if (sysfs_streq(buf, "1"))
> > +		sp_iop_standby_mode(iop);
> > +	return count;
> 
> So no matter what you write here, it will not return an error?
> 

I will modify it.

> That is not correct.
> 
> > +}
> > +
> > +static DEVICE_ATTR_RO(sp_iop_mailbox); static
> > +DEVICE_ATTR_RW(sp_iop_mode);
> > +
> > +static int  sp_iop_get_normal_code(struct device *dev, struct sp_iop
> > +*iop) {
> > +	const struct firmware *fw;
> > +	static const char file[] = "normal.bin";
> > +	unsigned int err, i;
> > +
> > +	err = request_firmware(&fw, file, dev);
> > +	if (err) {
> > +		dev_err(dev, "get bin file error\n");
> > +		return err;
> > +	}
> > +
> > +	for (i = 0; i < NORMAL_CODE_MAX_SIZE; i++) {
> > +		char temp;
> > +
> > +		temp = fw->data[i];
> > +		iop->iop_normal_code[i] = temp;
> > +	}
> > +	release_firmware(fw);
> > +	return err;
> > +}
> > +
> > +static int  sp_iop_get_standby_code(struct device *dev, struct sp_iop
> > +*iop) {
> > +	const struct firmware *fw;
> > +	static const char file[] = "standby.bin";
> > +	unsigned int err, i;
> > +
> > +	err = request_firmware(&fw, file, dev);
> > +	if (err) {
> > +		dev_err(dev, "get bin file error\n");
> > +		return err;
> > +	}
> > +
> > +	for (i = 0; i < STANDBY_CODE_MAX_SIZE; i++) {
> > +		char temp;
> > +
> > +		temp = fw->data[i];
> > +		iop->iop_standby_code[i] = temp;
> > +	}
> > +	release_firmware(fw);
> > +	return err;
> > +}
> > +
> > +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 IOP_SUCCESS;
> > +}
> > +
> > +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;
> > +	}
> > +	/* init */
> > +	mutex_init(&iop->write_lock);
> > +	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->iop_mem_start = mem_res.start;
> > +	iop->iop_mem_size = resource_size(&mem_res);
> > +
> > +	ret = sp_iop_get_normal_code(&pdev->dev, iop);
> > +	if (ret != 0) {
> > +		dev_err(&pdev->dev, "get normal code err=%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = sp_iop_get_standby_code(&pdev->dev, iop);
> > +	if (ret != 0) {
> > +		dev_err(&pdev->dev, "get standby code err=%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	sp_iop_normal_mode(iop);
> > +	platform_set_drvdata(pdev, iop);
> > +	device_create_file(&pdev->dev, &dev_attr_sp_iop_mailbox);
> > +	device_create_file(&pdev->dev, &dev_attr_sp_iop_mode);
> 
> You just raced with userspace and lost.  Set the default groups pointer of the
> misc device to your attribute group and then they will be automatically
> created for you.
> 
> 
> 
> 
> > +	iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node,
> "iop-wakeup", 0);
> > +	return 0;
> > +
> > +fail_kmalloc:
> > +	return ret;
> > +}
> > +
> > +static void sp_iop_platform_driver_shutdown(struct platform_device
> > +*pdev) {
> > +	struct sp_iop *iop = platform_get_drvdata(pdev);
> > +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> > +	unsigned int value;
> > +
> > +	sp_iop_standby_mode(iop);
> > +	mdelay(10);
> 
> Why sleep on shutdown?
> 

I need time to switch from normal.bin code to standby.bin code.				

> thanks,
> 
> greg k-h

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

[Greg KH]

On Mon, Jan 03, 2022 at 05:31:27AM +0000, Tony Huang 黃懷厚 wrote:
> Dear Greg:
> 
> > > IOP(8051) embedded inside SP7021 which is used as Processor for I/O
> > > control, monitor RTC interrupt 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 <tonyhuang.sunplus@gmail.com>
> > > ---
> > > Changes in v6:
> > >  - Added sysfs read/write description.
> > >  - Modify sysfs read function.
> > >  - Addressed comments from kernel test robot.
> > >
> > >  Documentation/ABI/testing/sysfs-platform-soc@B |  25 ++
> > >  MAINTAINERS                                    |   2 +
> > >  drivers/misc/Kconfig                           |  12 +
> > >  drivers/misc/Makefile                          |   1 +
> > >  drivers/misc/sunplus_iop.c                     | 476
> > +++++++++++++++++++++++++
> > >  5 files changed, 516 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-platform-soc@B
> > >  create mode 100644 drivers/misc/sunplus_iop.c
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-platform-soc@B
> > > b/Documentation/ABI/testing/sysfs-platform-soc@B
> > > new file mode 100644
> > > index 0000000..6272919
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-platform-soc@B
> > > @@ -0,0 +1,25 @@
> > > +What:		/sys/devices/platform/soc@B/9c000400.iop/sp_iop_mailbox
> > > +Date:		December 2021
> > > +KernelVersion:	5.16
> > > +Contact:	Tony Huang <tonyhuang.sunplus@gmail.com>
> > > +Description:
> > > +		Show 8051 mailbox0 data.
> > 
> > What format is the data in?
> > 
> 
> Unsigned short

So you are returning 16 bits, please describe the value as to what it
will contain and what it means.

And what exactly does "8051" mean here?  That is just some random
processor in the system, it needs to be described better please.

> > > +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.
> > 
> > I do not understand this sentence, what do you mean by it?  Can you provide a
> > link to where the files that are required are?  Why not include them in the
> > linux-firmware project?
> > 
> 
> 1.We will provide users with 8051 normal and standby 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			
> 2.Users can follow the operation steps to generate normal.bin and standby.bin.			
>   Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051			
>   26.5 How To Create 8051 bin file			

Please include stuff like this in the help text to make it more obvious.

> > > +struct regs_moon0 {
> > > +	u32 stamp;         /* 00 */
> > > +	u32 clken[10];     /* 01~10 */
> > > +	u32 gclken[10];    /* 11~20 */
> > > +	u32 reset[10];     /* 21~30 */
> > > +	u32 sfg_cfg_mode;  /* 31 */
> > 
> > What are these comments numbering?
> > 
> 
> regs_moon0 is Group 0 moon register.					
> The Group0 moon register range is 0x9c00000~0x9c00007F					
> /*00*/: 0x9c000000~0x9c000003					
> /*01~10*/:0x9c000004~0x9c00002b					
> /*11~20*/:0x9c00002c~0x9c000053					
> /*21~30*/:0x9c000054~0x9c00007b					
> /*31*/:0x9c00007c~0x9c00007f					

Ok, so the number is just the offset into a memory location?  Please be
specific.


> 
> > > +};
> > > +
> > > +struct regs_iop {
> > > +	u32 iop_control;/* 00 */
> > > +	u32 iop_reg1;/* 01 */
> > > +	u32 iop_bp;/* 02 */
> > > +	u32 iop_regsel;/* 03 */
> > > +	u32 iop_regout;/* 04 */
> > > +	u32 iop_reg5;/* 05 */
> > > +	u32 iop_resume_pcl;/* 06 */
> > > +	u32 iop_resume_pch;/* 07 */
> > > +	u32 iop_data0;/* 08 */
> > > +	u32 iop_data1;/* 09 */
> > > +	u32 iop_data2;/* 10 */
> > > +	u32 iop_data3;/* 11 */
> > > +	u32 iop_data4;/* 12 */
> > > +	u32 iop_data5;/* 13 */
> > > +	u32 iop_data6;/* 14 */
> > > +	u32 iop_data7;/* 15 */
> > > +	u32 iop_data8;/* 16 */
> > > +	u32 iop_data9;/* 17 */
> > > +	u32 iop_data10;/* 18 */
> > > +	u32 iop_data11;/* 19 */
> > > +	u32 iop_base_adr_l;/* 20 */
> > > +	u32 iop_base_adr_h;/* 21 */
> > > +	u32 memory_bridge_control;/* 22 */
> > > +	u32 iop_regmap_adr_l;/* 23 */
> > > +	u32 iop_regmap_adr_h;/* 24 */
> > > +	u32 iop_direct_adr;/* 25*/
> > > +	u32 reserved[6];/* 26~31 */
> > 
> > Same here, what are these numbers?
> > 
> > And why are they not lined up like the previous structure?
> > 
> 
> Sorry, I don't understand what you mean. Isn't this a struct?					

Your comments are not lined up the same way the structure above has
them.

> > > +	struct mutex write_lock;
> > > +	void *iop_regs;
> > > +	void *pmc_regs;
> > > +	void *moon0_regs;
> > 
> > Why void pointers?  You created structures above, use them!
> > 
> 
> Because I received "Reported-by: kernel test robot <lkp@intel.com>", warmming message. As follows:										
> sparse warnings: (new ones prefixed by >>)										
>    drivers/misc/sunplus_iop.c:94:39: sparse: sparse: cast removes address space '__iomem' of expression										
>    drivers/misc/sunplus_iop.c:95:43: sparse: sparse: cast removes address space '__iomem' of expression										
> >> drivers/misc/sunplus_iop.c:100:16: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void *p @@     got void [noderef] __iomem *[assigned] iop_kernel_base @@										
>    drivers/misc/sunplus_iop.c:100:16: sparse:     expected void *p										
>    drivers/misc/sunplus_iop.c:100:16: sparse:     got void [noderef] __iomem *[assigned] iop_kernel_base										

Ah, so these are actual pointers to the iomem memory?  Or pointers to
the structures you have copied out?  It is not obvious.

> > > +	reg = readl(&p_moon0_reg->reset[0]);
> > > +	reg |= 0x10;
> > > +	writel(reg, (&p_moon0_reg->reset[0]));
> > > +	reg &= ~(0x10);
> > > +	writel(reg, (&p_moon0_reg->reset[0]));
> > > +
> > > +	writel(0x00ff0085, (iop->moon0_regs + 32 * 4 * 1 + 4 * 1));
> > > +
> > > +	reg = readl(iop->moon0_regs + 32 * 4 * 1 + 4 * 2);
> > > +	reg |= 0x08000800;
> > > +	writel(reg, (iop->moon0_regs + 32 * 4 * 1 + 4 * 2));
> > > +
> > > +	reg = readl(&p_iop_reg->iop_control);
> > > +	reg |= 0x0200;//disable watchdog event reset IOP
> > > +	writel(reg, &p_iop_reg->iop_control);
> > > +
> > > +	reg = (iop->iop_mem_start & 0xFFFF);
> > > +	writel(reg, &p_iop_reg->iop_base_adr_l);
> > > +	reg = (iop->iop_mem_start >> 16);
> > > +	writel(reg, &p_iop_reg->iop_base_adr_h);
> > > +
> > > +	reg = readl(&p_iop_reg->iop_control);
> > > +	reg &= ~(0x01);
> > > +	writel(reg, &p_iop_reg->iop_control);
> > > +
> > > +	writel(WAKEUP_PIN, &p_iop_reg->iop_data0);
> > > +	writel(iop->gpio_wakeup, &p_iop_reg->iop_data1);
> > > +
> > > +	ret = readl_poll_timeout(&p_iop_reg->iop_data2, value,
> > > +				 (value & IOP_READY) == IOP_READY, 1000, 10000);
> > > +	if (ret) {
> > > +		dev_err(dev, "timed out\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	writel(RISC_READY, &p_iop_reg->iop_data2);
> > > +	writel(0x00, &p_iop_reg->iop_data5);
> > > +	writel(0x60, &p_iop_reg->iop_data6);
> > > +
> > > +	ret = readl_poll_timeout(&p_iop_reg->iop_data7, value,
> > > +				 value == 0xaaaa, 1000, 10000);
> > > +	if (ret) {
> > > +		dev_err(dev, "timed out\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	writel(0xdd, &p_iop_reg->iop_data1);//8051 bin file call Ultra low
> > function.
> > > +	mdelay(10);
> > 
> > Where did 10 come from?  How do you know that is correct?
> > 
> 
> I need time to move stanby.bin code 16K data from SDRAM to 8051's icache.		
> 10msec should be enough		

Please document it, as it looks like the data was already sent so there
should not be any need for a delay on the Linux side.

Or do a read which will ensure that the data is there, right?

> > > +static void sp_iop_platform_driver_shutdown(struct platform_device
> > > +*pdev) {
> > > +	struct sp_iop *iop = platform_get_drvdata(pdev);
> > > +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> > > +	unsigned int value;
> > > +
> > > +	sp_iop_standby_mode(iop);
> > > +	mdelay(10);
> > 
> > Why sleep on shutdown?
> > 
> 
> I need time to switch from normal.bin code to standby.bin code.				

Then why does the function call not wait until that happens?  Why wait
here?

thanks,

greg k-h

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

[Tony Huang 黃懷厚]

Dear Greg:

> > > > IOP(8051) embedded inside SP7021 which is used as Processor for
> > > > I/O control, monitor RTC interrupt 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 <tonyhuang.sunplus@gmail.com>
> > > > ---
> > > > Changes in v6:
> > > >  - Added sysfs read/write description.
> > > >  - Modify sysfs read function.
> > > >  - Addressed comments from kernel test robot.
> > > >
> > > >  Documentation/ABI/testing/sysfs-platform-soc@B |  25 ++
> > > >  MAINTAINERS                                    |   2 +
> > > >  drivers/misc/Kconfig                           |  12 +
> > > >  drivers/misc/Makefile                          |   1 +
> > > >  drivers/misc/sunplus_iop.c                     | 476
> > > +++++++++++++++++++++++++
> > > >  5 files changed, 516 insertions(+)  create mode 100644
> > > > Documentation/ABI/testing/sysfs-platform-soc@B
> > > >  create mode 100644 drivers/misc/sunplus_iop.c
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-platform-soc@B
> > > > b/Documentation/ABI/testing/sysfs-platform-soc@B
> > > > new file mode 100644
> > > > index 0000000..6272919
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-platform-soc@B
> > > > @@ -0,0 +1,25 @@
> > > > +What:
> 	/sys/devices/platform/soc@B/9c000400.iop/sp_iop_mailbox
> > > > +Date:		December 2021
> > > > +KernelVersion:	5.16
> > > > +Contact:	Tony Huang <tonyhuang.sunplus@gmail.com>
> > > > +Description:
> > > > +		Show 8051 mailbox0 data.
> > >
> > > What format is the data in?
> > >
> >
> > Unsigned short
> 
> So you are returning 16 bits, please describe the value as to what it will
> contain and what it means.
> 
> And what exactly does "8051" mean here?  That is just some random
> processor in the system, it needs to be described better please.
> 
> > > > +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.
> > >
> > > I do not understand this sentence, what do you mean by it?  Can you
> > > provide a link to where the files that are required are?  Why not
> > > include them in the linux-firmware project?
> > >
> >
> > 1.We will provide users with 8051 normal and standby 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
> 
> > 2.Users can follow the operation steps to generate normal.bin and
> standby.bin.
> >   Path:
> https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051
> 
> >   26.5 How To Create 8051 bin file
> 
> Please include stuff like this in the help text to make it more obvious.
> 

OK. I will add in help text.

> > > > +struct regs_moon0 {
> > > > +	u32 stamp;         /* 00 */
> > > > +	u32 clken[10];     /* 01~10 */
> > > > +	u32 gclken[10];    /* 11~20 */
> > > > +	u32 reset[10];     /* 21~30 */
> > > > +	u32 sfg_cfg_mode;  /* 31 */
> > >
> > > What are these comments numbering?
> > >
> >
> > regs_moon0 is Group 0 moon register.
> > The Group0 moon register range is 0x9c00000~0x9c00007F
> 
> > /*00*/: 0x9c000000~0x9c000003
> > /*01~10*/:0x9c000004~0x9c00002b
> > /*11~20*/:0x9c00002c~0x9c000053
> > /*21~30*/:0x9c000054~0x9c00007b
> > /*31*/:0x9c00007c~0x9c00007f
> 
> Ok, so the number is just the offset into a memory location?  Please be
> specific.
> 

OK, I will modify it.

> 
> >
> > > > +};
> > > > +
> > > > +struct regs_iop {
> > > > +	u32 iop_control;/* 00 */
> > > > +	u32 iop_reg1;/* 01 */
> > > > +	u32 iop_bp;/* 02 */
> > > > +	u32 iop_regsel;/* 03 */
> > > > +	u32 iop_regout;/* 04 */
> > > > +	u32 iop_reg5;/* 05 */
> > > > +	u32 iop_resume_pcl;/* 06 */
> > > > +	u32 iop_resume_pch;/* 07 */
> > > > +	u32 iop_data0;/* 08 */
> > > > +	u32 iop_data1;/* 09 */
> > > > +	u32 iop_data2;/* 10 */
> > > > +	u32 iop_data3;/* 11 */
> > > > +	u32 iop_data4;/* 12 */
> > > > +	u32 iop_data5;/* 13 */
> > > > +	u32 iop_data6;/* 14 */
> > > > +	u32 iop_data7;/* 15 */
> > > > +	u32 iop_data8;/* 16 */
> > > > +	u32 iop_data9;/* 17 */
> > > > +	u32 iop_data10;/* 18 */
> > > > +	u32 iop_data11;/* 19 */
> > > > +	u32 iop_base_adr_l;/* 20 */
> > > > +	u32 iop_base_adr_h;/* 21 */
> > > > +	u32 memory_bridge_control;/* 22 */
> > > > +	u32 iop_regmap_adr_l;/* 23 */
> > > > +	u32 iop_regmap_adr_h;/* 24 */
> > > > +	u32 iop_direct_adr;/* 25*/
> > > > +	u32 reserved[6];/* 26~31 */
> > >
> > > Same here, what are these numbers?
> > >
> > > And why are they not lined up like the previous structure?
> > >
> >
> > Sorry, I don't understand what you mean. Isn't this a struct?
> 
> 
> Your comments are not lined up the same way the structure above has them.
> 

OK, I will modify it.

> > > > +	struct mutex write_lock;
> > > > +	void *iop_regs;
> > > > +	void *pmc_regs;
> > > > +	void *moon0_regs;
> > >
> > > Why void pointers?  You created structures above, use them!
> > >
> >
> > Because I received "Reported-by: kernel test robot <lkp@intel.com>",
> warmming message. As follows:
> > sparse warnings: (new ones prefixed by >>)
> 
> >    drivers/misc/sunplus_iop.c:94:39: sparse: sparse: cast removes address
> space '__iomem' of expression
> >    drivers/misc/sunplus_iop.c:95:43: sparse: sparse: cast removes address
> space '__iomem' of expression
> > >> drivers/misc/sunplus_iop.c:100:16: sparse: sparse: incorrect type in
> argument 1 (different address spaces) @@     expected void *p @@
> got void [noderef] __iomem *[assigned] iop_kernel_base @@
> 
> >    drivers/misc/sunplus_iop.c:100:16: sparse:     expected void *p
> 
> >    drivers/misc/sunplus_iop.c:100:16: sparse:     got void [noderef]
> __iomem *[assigned] iop_kernel_base
> 
> 
> Ah, so these are actual pointers to the iomem memory?  Or pointers to the
> structures you have copied out?  It is not obvious.
> 
> > > > +	reg = readl(&p_moon0_reg->reset[0]);
> > > > +	reg |= 0x10;
> > > > +	writel(reg, (&p_moon0_reg->reset[0]));
> > > > +	reg &= ~(0x10);
> > > > +	writel(reg, (&p_moon0_reg->reset[0]));
> > > > +
> > > > +	writel(0x00ff0085, (iop->moon0_regs + 32 * 4 * 1 + 4 * 1));
> > > > +
> > > > +	reg = readl(iop->moon0_regs + 32 * 4 * 1 + 4 * 2);
> > > > +	reg |= 0x08000800;
> > > > +	writel(reg, (iop->moon0_regs + 32 * 4 * 1 + 4 * 2));
> > > > +
> > > > +	reg = readl(&p_iop_reg->iop_control);
> > > > +	reg |= 0x0200;//disable watchdog event reset IOP
> > > > +	writel(reg, &p_iop_reg->iop_control);
> > > > +
> > > > +	reg = (iop->iop_mem_start & 0xFFFF);
> > > > +	writel(reg, &p_iop_reg->iop_base_adr_l);
> > > > +	reg = (iop->iop_mem_start >> 16);
> > > > +	writel(reg, &p_iop_reg->iop_base_adr_h);
> > > > +
> > > > +	reg = readl(&p_iop_reg->iop_control);
> > > > +	reg &= ~(0x01);
> > > > +	writel(reg, &p_iop_reg->iop_control);
> > > > +
> > > > +	writel(WAKEUP_PIN, &p_iop_reg->iop_data0);
> > > > +	writel(iop->gpio_wakeup, &p_iop_reg->iop_data1);
> > > > +
> > > > +	ret = readl_poll_timeout(&p_iop_reg->iop_data2, value,
> > > > +				 (value & IOP_READY) == IOP_READY, 1000, 10000);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "timed out\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	writel(RISC_READY, &p_iop_reg->iop_data2);
> > > > +	writel(0x00, &p_iop_reg->iop_data5);
> > > > +	writel(0x60, &p_iop_reg->iop_data6);
> > > > +
> > > > +	ret = readl_poll_timeout(&p_iop_reg->iop_data7, value,
> > > > +				 value == 0xaaaa, 1000, 10000);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "timed out\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	writel(0xdd, &p_iop_reg->iop_data1);//8051 bin file call Ultra
> > > > +low
> > > function.
> > > > +	mdelay(10);
> > >
> > > Where did 10 come from?  How do you know that is correct?
> > >
> >
> > I need time to move stanby.bin code 16K data from SDRAM to 8051's icache.
> 
> > 10msec should be enough
> 
> Please document it, as it looks like the data was already sent so there should
> not be any need for a delay on the Linux side.
> 
> Or do a read which will ensure that the data is there, right?
> 

Let me explain:
1.
writel(0xdd, &p_iop_reg->iop_data1);//8051 bin file call Ultra low function < -----when system linux kernel wrtie mailbox1 iop_data1=0xdd.
8051's standby code see mailbox1=0xdd.
I will move stanby.bin code 16K data from SDRAM to 8051's icache in 8051's standby code.
I will turn off power through the 8051 register(DEF_PWR_EN_0=0) in 8051's standby code.
2.
mdelay(10);<-------	When the execution is here, the system linux kernel is about to be powered off	
The purpose: Do not let the system linux kernel continue to run other programs

> > > > +static void sp_iop_platform_driver_shutdown(struct
> > > > +platform_device
> > > > +*pdev) {
> > > > +	struct sp_iop *iop = platform_get_drvdata(pdev);
> > > > +	struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> > > > +	unsigned int value;
> > > > +
> > > > +	sp_iop_standby_mode(iop);
> > > > +	mdelay(10);
> > >
> > > Why sleep on shutdown?
> > >
> >
> > I need time to switch from normal.bin code to standby.bin code.
> 
> 
> Then why does the function call not wait until that happens?  Why wait here?
> 

sp_iop_standby_mode(iop);	<--- I need ensure to switch from normal.bin code to standby.bin code.			
mdelay(10);	<--- I will remove this delay time. I will read the data to confirm to replace the delay time			

Thanks

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

[Rob Herring]

On Thu, 30 Dec 2021 20:51:44 +0800, Tony Huang wrote:
> Add iop yaml file for Sunplus SP7021
> 
> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> ---
> Changes in v6:
>  -Modify incorrect name of gpio. Added wakeup-gpios pin for 8051.
> 
>  .../devicetree/bindings/misc/sunplus-iop.yaml      | 76 ++++++++++++++++++++++
>  MAINTAINERS                                        |  5 ++
>  2 files changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/sunplus-iop.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>