Dear Krzysztof: I will modify the code to follow your comments Thanks > Subject: Re: [PATCH v10 2/2] misc: Add iop driver for Sunplus SP7021 > > On 07/03/2022 06:25, Tony Huang wrote: > > This driver is load 8051 bin code. > > Processor for I/O control: > > SP7021 has its own GPIO device driver. > > The user can configure the gpio pin for 8051 use. > > The 8051 support wake-up with IR key after system poweroff. > > > > Monitor RTC interrupt: > > SP7021 system has its own RTC device driver (rtc-sunplus.c). > > The user can set the RTC wake-up time through rtc-sunplus.c. > > The IOP code does not need to increase the RTC subsystem function, > > set the RTC register. When the linux kernel system is poweroff. > > Only the 8051 CPU has power and can monitor the RTC wake-up interrupt. > > > > PMC in power management purpose: > > Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff. > > When the poweroff command is executed. > > The 8051 has a register to control the power-on and power-off of the > > system. If you turn off the power through the 8051 > > register(DEF_PWR_EN_0=0). The current measured by the circuit board is > > 0.4mA only. In order to save power. > > > > Signed-off-by: Tony Huang <tonyhuang.sunplus@xxxxxxxxx> > > --- > > Changes in v10: > > - Added sp_iop_poweroff function for poweroff command. > > > > MAINTAINERS | 1 + > > drivers/misc/Kconfig | 36 ++++ > > drivers/misc/Makefile | 1 + > > drivers/misc/sunplus_iop.c | 438 > > +++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 476 insertions(+) > > create mode 100644 drivers/misc/sunplus_iop.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS index 6f336c9..11ecefa 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -18246,6 +18246,7 @@ SUNPLUS IOP DRIVER > > M: Tony Huang <tonyhuang.sunplus@xxxxxxxxx> > > S: Maintained > > F: Documentation/devicetree/bindings/misc/sunplu-iop.yaml > > +F: drivers/misc/sunplus_iop.c > > > > SUPERH > > M: Yoshinori Sato <ysato@xxxxxxxxxxxxxxxxxxxx> > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index > > 0f5a49f..3106f15 100644 > > --- a/drivers/misc/Kconfig > > +++ b/drivers/misc/Kconfig > > @@ -470,6 +470,42 @@ config HISI_HIKEY_USB > > switching between the dual-role USB-C port and the USB-A host ports > > using only one USB controller. > > > > +config SUNPLUS_IOP > > + tristate "Sunplus IOP support" > > + default ARCH_SUNPLUS > > + help > > + This driver is load 8051 bin code. > > + Processor for I/O control: > > + SP7021 has its own GPIO device driver. > > + The user can configure the gpio pin for 8051 use. > > + 8051 support wake-up with IR key after system poweroff. > > + > > + Monitor RTC interrupt: > > + SP7021 system has its own RTC device driver (rtc-sunplus.c). > > + The user can set the RTC wake-up time through rtc-sunplus.c. > > + The IOP code does not need to increase the RTC subsystem function, > > + set the RTC register. When the linux kernel system is poweroff. > > + Only the 8051 CPU has power and can monitor the RTC wake-up > interrupt. > > + > > + PMC in power management purpose: > > + Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff. > > + When the poweroff command is executed. > > + The 8051 has a register to control the power-on and power-off of the > system. > > + If you turn off the power through the 8051 register(DEF_PWR_EN_0=0), > > + The current measured by the circuit board is 0.4mA only. In order to > save power. > > + > > + The IOP core is DQ8051, so also named IOP8051. > > + Need Install DQ8051, The DQ8051 bin file generated by keil C. > > + We will provide users with 8051 normal and 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 > > + 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..03301b4 > > --- /dev/null > > +++ b/drivers/misc/sunplus_iop.c > > @@ -0,0 +1,438 @@ > > +// 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/of_platform.h> > > +#include <linux/of_address.h> > > +#include <linux/of_gpio.h> > > + > > Except comments from Arnd also: > > > +enum IOP_Status_e { > > + IOP_SUCCESS, /* successful */ > > + IOP_ERR_IOP_BUSY, /* IOP is busy */ > > +}; > > please do not redefine true/false or ERRNO. > > > + > > +/* moon0 register offset */ > > +#define IOP_CLKEN0 0x04 > > +#define IOP_RESET0 0x54 > > + > > +/* IOP register offset */ > > +#define IOP_CONTROL 0x00 > > +#define IOP_DATA0 0x20 > > +#define IOP_DATA1 0x24 > > +#define IOP_DATA2 0x28 > > +#define IOP_DATA3 0x2c > > +#define IOP_DATA4 0x30 > > +#define IOP_DATA5 0x34 > > +#define IOP_DATA6 0x38 > > +#define IOP_DATA7 0x3c > > +#define IOP_DATA8 0x40 > > +#define IOP_DATA9 0x44 > > +#define IOP_DATA10 0x48 > > +#define IOP_DATA11 0x4c > > +#define IOP_BASE_ADR_L 0x50 > > +#define IOP_BASE_ADR_H 0x54 > > + > > +/* PMC register offset */ > > +#define IOP_PMC_TIMER 0x00 > > +#define IOP_PMC_CTRL 0x04 > > +#define IOP_XTAL27M_PASSWORD_I 0x08 > > +#define IOP_XTAL27M_PASSWORD_II 0x0c > > +#define IOP_XTAL32K_PASSWORD_I 0x10 > > +#define IOP_XTAL32K_PASSWORD_II 0x14 > > +#define IOP_CLK27M_PASSWORD_I 0x18 > > +#define IOP_CLK27M_PASSWORD_II 0x1c > > +#define IOP_PMC_TIMER2 0x20 > > + > > +#define NORMAL_CODE_MAX_SIZE 0X1000 /* Max size of normal.bin > that can be received */ > > +#define STANDBY_CODE_MAX_SIZE 0x4000 /* Max size of standby.bin > that can be received */ > > Empty line. > > > +struct sp_iop { > > + struct device *dev; > > + struct mutex write_lock; /* avoid parallel access */ > > This comment is useless. It is obvious that mutex is used to avoid parallel > access, what else could it be used? > > Instead, you need to describe which members/variables/parts of code are > protected with the mutex. > > > + void __iomem *iop_regs; > > + void __iomem *pmc_regs; > > + void __iomem *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]; > > Why not using firmware? include/linux/firmware.h? Do not reimplement > existing solutions... > > > + resource_size_t iop_mem_start; > > + resource_size_t iop_mem_size; > > + unsigned char bin_code_mode; > > +}; > > + > > +static struct sp_iop *iop_poweroff; > > + > > +static void sp_iop_run_normal_code(struct sp_iop *iop) { > > + void __iomem *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, iop->moon0_regs + IOP_CLKEN0); > > + > > + reg = readl(iop->iop_regs + IOP_CONTROL); > > + reg |= 0x01; > > + writel(reg, iop->iop_regs + IOP_CONTROL); > > + > > + reg = readl(iop->iop_regs + IOP_CONTROL); > > + reg &= ~(0x8000); > > + writel(reg, iop->iop_regs + IOP_CONTROL); > > + > > + reg = readl(iop->iop_regs + IOP_CONTROL); > > + reg |= 0x0200;// disable watchdog event reset IOP > > + writel(reg, iop->iop_regs + IOP_CONTROL); > > + > > + reg = (iop->iop_mem_start & 0xFFFF); > > + writel(reg, iop->iop_regs + IOP_BASE_ADR_L); > > + reg = (iop->iop_mem_start >> 16); > > + writel(reg, iop->iop_regs + IOP_BASE_ADR_H); > > + > > + reg = readl(iop->iop_regs + IOP_CONTROL); > > + reg &= ~(0x01); > > + writel(reg, iop->iop_regs + IOP_CONTROL); > > + iop->bin_code_mode = 0; > > +} > > + > > +static void sp_iop_run_standby_code(struct sp_iop *iop) { > > + void __iomem *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, iop->moon0_regs + IOP_CLKEN0); > > + > > + reg = readl(iop->iop_regs + IOP_CONTROL); > > + reg |= 0x01; > > + writel(reg, iop->iop_regs + IOP_CONTROL); > > + > > + reg = readl(iop->iop_regs + IOP_CONTROL); > > + reg &= ~(0x8000); > > + writel(reg, iop->iop_regs + IOP_CONTROL); > > + > > + reg = readl(iop->iop_regs + IOP_CONTROL); > > + reg |= 0x0200;// disable watchdog event reset IOP > > + writel(reg, iop->iop_regs + IOP_CONTROL); > > + > > + reg = (iop->iop_mem_start & 0xFFFF); > > + writel(reg, iop->iop_regs + IOP_BASE_ADR_L); > > + reg = (iop->iop_mem_start >> 16); > > + writel(reg, iop->iop_regs + IOP_BASE_ADR_H); > > + > > + reg = readl(iop->iop_regs + IOP_CONTROL); > > + reg &= ~(0x01); > > + writel(reg, iop->iop_regs + IOP_CONTROL); > > + iop->bin_code_mode = 1; > > +} > > + > > +/* 8051 informs linux kerenl. 8051 has been switched to standby.bin code. > */ > > +#define IOP_READY 0x0004 > > +#define RISC_READY 0x0008 > > + > > +/* System linux kernel tells 8051 which gpio pin to wake-up through. */ > > +#define WAKEUP_PIN 0xFE02 > > + > > +/* System linux kernel tells 8051 to execute S1 or S3 mode. */ > > +#define S1 0x5331 > > +#define S3 0x5333 > > Keep defines together, so beginning of file. > > > + > > +static int sp_iop_s3mode(struct device *dev, struct sp_iop *iop) { > > + unsigned int reg; > > + int ret, value; > > + > > + /* PMC set */ > > + writel(0x00010001, iop->pmc_regs + IOP_PMC_TIMER); > > + reg = readl(iop->pmc_regs + IOP_PMC_CTRL); > > + /* disable system reset PMC, enalbe power down IOP Domain, enable > gating clock 27Mhz */ > > + reg |= 0x23; > > + writel(reg, iop->pmc_regs + IOP_PMC_CTRL); > > + > > + writel(0x55aa00ff, iop->pmc_regs + IOP_XTAL27M_PASSWORD_I); > > + writel(0x00ff55aa, iop->pmc_regs + IOP_XTAL27M_PASSWORD_II); > > + writel(0xaa00ff55, iop->pmc_regs + IOP_XTAL32K_PASSWORD_I); > > + writel(0xff55aa00, iop->pmc_regs + IOP_XTAL32K_PASSWORD_II); > > + writel(0xaaff0055, iop->pmc_regs + IOP_CLK27M_PASSWORD_I); > > + writel(0x5500aaff, iop->pmc_regs + IOP_CLK27M_PASSWORD_II); > > + writel(0x01000100, iop->pmc_regs + IOP_PMC_TIMER2); > > + > > + /* IOP Hardware IP reset */ > > + reg = readl(iop->moon0_regs + IOP_RESET0); > > + reg |= 0x10; > > + writel(reg, (iop->moon0_regs + IOP_RESET0)); > > + reg &= ~(0x10); > > + writel(reg, (iop->moon0_regs + IOP_RESET0)); > > + > > + 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)); > > + > > + writel(WAKEUP_PIN, iop->iop_regs + IOP_DATA0); > > + writel(iop->gpio_wakeup, iop->iop_regs + IOP_DATA1); > > + > > + ret = readl_poll_timeout(iop->iop_regs + IOP_DATA2, value, > > + (value & IOP_READY) == IOP_READY, 1000, 10000); > > + if (ret) { > > + dev_err(dev, "timed out\n"); > > + return ret; > > + } > > + > > + reg = RISC_READY; > > + writel(reg, iop->iop_regs + IOP_DATA2); > > + reg = 0x0000; > > + writel(reg, iop->iop_regs + IOP_DATA5); > > + reg = 0x0060; > > + writel(reg, iop->iop_regs + IOP_DATA6); > > + > > + ret = readl_poll_timeout(iop->iop_regs + IOP_DATA7, value, > > + value == 0xaaaa, 1000, 10000); > > + if (ret) { > > + dev_err(dev, "timed out\n"); > > + return ret; > > + } > > + > > + /* 8051 bin file call Ultra low function. */ > > + writel(0xdd, iop->iop_regs + IOP_DATA1); > > + /* > > + * When the execution is here, the system linux kernel > > + * is about to be powered off > > + * The purpose of mdelay(10): Do not let the system linux > > + * kernel continue to run other programs. > > + */ > > + mdelay(10); > > + return 0; > > +} > > + > > +static int sp_iop_s1mode(struct device *dev, struct sp_iop *iop) { > > + int ret, value; > > + > > + ret = readl_poll_timeout(iop->iop_regs + IOP_DATA2, value, > > + (value & IOP_READY) == IOP_READY, 1000, 10000); > > + if (ret) { > > + dev_err(dev, "timed out\n"); > > + return ret; > > + } > > + > > + writel(RISC_READY, iop->iop_regs + IOP_DATA2); > > + writel(0x0000, iop->iop_regs + IOP_DATA5); > > + writel(0x0060, iop->iop_regs + IOP_DATA6); > > + > > + ret = readl_poll_timeout(iop->iop_regs + IOP_DATA7, value, > > + value == 0xaaaa, 1000, 10000); > > Hundreds of magical constants... Almost all of these should be defined or > described. > > > + if (ret) { > > + dev_err(dev, "timed out\n"); > > + return ret; > > + } > > + > > + /* 8051 bin file call S1_mode function. */ > > + writel(0xee, iop->iop_regs + IOP_DATA1); > > + /* > > + * When the execution is here, the system linux kernel > > + * is about to be powered off > > + * The purpose of mdelay(10): Do not let the system linux > > + * kernel continue to run other programs. > > + */ > > + mdelay(10); > > + return 0; > > +} > > + > > +static 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 void sp_iop_poweroff(void) > > +{ > > + struct sp_iop *iop = iop_poweroff; > > + unsigned int ret, value; > > + > > + value = readl(iop->iop_regs + IOP_DATA11); > > + sp_iop_run_standby_code(iop); > > + > > + ret = readl_poll_timeout(iop->iop_regs + IOP_DATA0, value, > > + value == 0x2222, 1000, 100000); > > + if (ret) > > + dev_warn(iop->dev, "timed out\n"); > > + > > + if (value == S1) > > + sp_iop_s1mode(iop->dev, iop); > > + else > > + sp_iop_s3mode(iop->dev, iop); > > +} > > + > > +static int sp_iop_get_resources(struct platform_device *pdev, struct > > +sp_iop *p_sp_iop_info) { > > + struct resource *r; > > + > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop"); > > + p_sp_iop_info->iop_regs = devm_ioremap_resource(&pdev->dev, r); > > + if (IS_ERR(p_sp_iop_info->iop_regs)) { > > + dev_err(&pdev->dev, "ioremap fail\n"); > > + return PTR_ERR(p_sp_iop_info->iop_regs); > > + } > > + > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, > "iop_pmc"); > > + p_sp_iop_info->pmc_regs = devm_ioremap_resource(&pdev->dev, r); > > + if (IS_ERR(p_sp_iop_info->pmc_regs)) { > > + dev_err(&pdev->dev, "ioremap fail\n"); > > + return PTR_ERR(p_sp_iop_info->pmc_regs); > > + } > > + > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon0"); > > + p_sp_iop_info->moon0_regs = devm_ioremap_resource(&pdev->dev, r); > > + if (IS_ERR(p_sp_iop_info->moon0_regs)) { > > + dev_err(&pdev->dev, "ioremap fail\n"); > > + return PTR_ERR(p_sp_iop_info->moon0_regs); > > + } > > + return 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; > > Wrong spacing before '='. > > > + goto fail_kmalloc; > > + } > > + /* init */ > > Remove useless comments. > > > + mutex_init(&iop->write_lock); > > Where are lock/unlock operations? What is this all code? This looks like > half-baked solution... > > > + ret = sp_iop_get_resources(pdev, iop); > > + > > + // Get reserve address > > Keep consistent style. For comments /* is preferred. But definitely do not mix > one with another. > > This code is difficult to read. > > > + memnp = of_parse_phandle(pdev->dev.of_node, "memory-region", 0); > > + if (!memnp) { > > + dev_err(&pdev->dev, "no memory-region node\n"); > > + return -EINVAL; > > + } > > + > > + rc = of_address_to_resource(memnp, 0, &mem_res); > > + of_node_put(memnp); > > + if (rc) { > > + dev_err(&pdev->dev, "failed to translate memory-region to a > resource\n"); > > + return -EINVAL; > > + } > > + > > + iop->dev = &pdev->dev; > > + iop->iop_mem_start = mem_res.start; > > + iop->iop_mem_size = resource_size(&mem_res); > > + > > + /* > > + * 8051 has two bin files, normal.bin and standby.bin in rootfs. > > + * Normally, 8051 executes normal.bin code. Normal.bin code size can > exceed 16K. > > + * When system linux kernel is shutdown, 8051 is to execute standby.bin > code. > > + * Standby.bin code cannot exceed 16K bytes. Because 8051 Icache size is > 16k. > > + * 8051 runs with standby.bin code in the Icache before the system(linux > kernel) > > + * is poweroff. > > + */ > > + 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_run_normal_code(iop); > > + platform_set_drvdata(pdev, iop); > > + iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node, > "iop-wakeup", 0); > > + iop_poweroff = iop; > > + pm_power_off = sp_iop_poweroff; > > Blank line. Before every such return. Please, open some recent drivers and look > at their code style. > > > + return 0; > > + > > +fail_kmalloc: > > + return ret; > > +} > > + > > +static int sp_iop_remove(struct platform_device *pdev) { > > + pm_power_off = NULL; > > + return 0; > > +} > > + > > +static const struct of_device_id sp_iop_of_match[] = { > > + { .compatible = "sunplus,sp7021-iop" }, > > + { /* sentinel */ }, > > +}; > > + > > +MODULE_DEVICE_TABLE(of, sp_iop_of_match); > > + > > +static struct platform_driver sp_iop_platform_driver = { > > + .probe = sp_iop_platform_driver_probe, > > + .remove = sp_iop_remove, > > + .driver = { > > + .name = "sunplus,sp7021-iop", > > + .of_match_table = sp_iop_of_match, > > + } > > +}; > > + > > +module_platform_driver(sp_iop_platform_driver); > > + > > +MODULE_AUTHOR("Tony Huang <tonyhuang.sunplus@xxxxxxxxx>"); > > +MODULE_DESCRIPTION("Sunplus IOP Driver"); MODULE_LICENSE("GPL > v2"); > > > Best regards, > Krzysztof
