On 12/03/2022 17:16, Tony Huang wrote: > This driver is load 8051 bin code. > The IOP core is DQ8051, so also named IOP8051. > Need Install DQ8051, The DQ8051 bin file generated by keil C. > We will provide users with 8051 normal and power off source code. > Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/ > How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source- > files-for-IOP > Users can follow the operation steps to generate normal.bin and > poweroff.bin. > Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338 > /26.+IOP8051 26.5?How To Create 8051 bin file > > PMC in power management purpose: > Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff. > When the power off command is executed. > The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and > power-off of the system. > > Signed-off-by: Tony Huang <tonyhuang.sunplus@xxxxxxxxx> > --- > Changes in v11: > - Addressed comments from Arnd Bergmann. How did you address Arnd's comments about splitting the driver to proper parts? drivers/clk and drivers/power/reset? > - Addressed comments from krzysztof. > > MAINTAINERS | 1 + > drivers/misc/Kconfig | 23 +++ > drivers/misc/Makefile | 1 + > drivers/misc/sunplus_iop.c | 411 +++++++++++++++++++++++++++++++++++++++++++++ The driver looks like SoC specific driver. Why did you put it in drivers/misc/, not in usual place - drivers/soc/? sp_iop_poweroff is still here. > 4 files changed, 436 insertions(+) > create mode 100644 drivers/misc/sunplus_iop.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index d64c8ed..c282e95 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -18246,6 +18246,7 @@ SUNPLUS IOP DRIVER > M: Tony Huang <tonyhuang.sunplus@xxxxxxxxx> > S: Maintained > F: Documentation/devicetree/bindings/misc/sunplus,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..e5f32d8 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -470,6 +470,29 @@ config HISI_HIKEY_USB > switching between the dual-role USB-C port and the USB-A host ports > using only one USB controller. > > +config SUNPLUS_IOP > + tristate "Sunplus IOP support" > + default ARCH_SUNPLUS > + help > + This driver is load 8051 bin code. > + The IOP core is DQ8051, so also named IOP8051. > + Need Install DQ8051, The DQ8051 bin file generated by keil C. > + We will provide users with 8051 normal and power off source code. > + Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/ > + How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source-files-for-IOP > + Users can follow the operation steps to generate normal.bin and poweroff.bin. > + Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051 > + 26.5?How To Create 8051 bin file > + > + PMC in power management purpose: > + Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff. > + When the power off command is executed. > + The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and > + power-off of the system. > + > + This driver can also be built as a module. If so, the module > + will be called sunplus_iop. > + > source "drivers/misc/c2port/Kconfig" > source "drivers/misc/eeprom/Kconfig" > source "drivers/misc/cb710/Kconfig" > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index a086197..eafeab6 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -52,6 +52,7 @@ obj-$(CONFIG_DW_XDATA_PCIE) += dw-xdata-pcie.o > obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o > obj-$(CONFIG_OCXL) += ocxl/ > obj-$(CONFIG_BCM_VK) += bcm-vk/ > +obj-$(CONFIG_SUNPLUS_IOP) += sunplus_iop.o > obj-y += cardreader/ > obj-$(CONFIG_PVPANIC) += pvpanic/ > obj-$(CONFIG_HABANA_AI) += habanalabs/ > diff --git a/drivers/misc/sunplus_iop.c b/drivers/misc/sunplus_iop.c > new file mode 100644 > index 0000000..5bdce5e > --- /dev/null > +++ b/drivers/misc/sunplus_iop.c > @@ -0,0 +1,411 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * The IOP driver for Sunplus SP7021 > + * > + * Copyright (C) 2021 Sunplus Technology Inc. > + * > + * All Rights Reserved. > + */ > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/dma-mapping.h> > +#include <linux/firmware.h> > +#include <linux/iopoll.h> > +#include <linux/module.h> > +#include <linux/of_platform.h> > +#include <linux/of_address.h> > +#include <linux/of_gpio.h> > + > +/* IOP register offset */ > +#define IOP_CONTROL 0x00 > +#define IOP_DATA0 0x20 > +#define IOP_DATA1 0x24 > +#define IOP_DATA2 0x28 > +#define IOP_DATA3 0x2c > +#define IOP_DATA4 0x30 > +#define IOP_DATA5 0x34 > +#define IOP_DATA6 0x38 > +#define IOP_DATA7 0x3c > +#define IOP_DATA8 0x40 > +#define IOP_DATA9 0x44 > +#define IOP_DATA10 0x48 > +#define IOP_DATA11 0x4c > +#define IOP_BASE_ADR_L 0x50 > +#define IOP_BASE_ADR_H 0x54 > + > +/* PMC register offset */ > +#define IOP_PMC_TIMER 0x00 > +#define IOP_PMC_CTRL 0x04 > +#define IOP_XTAL27M_PASSWORD_I 0x08 > +#define IOP_XTAL27M_PASSWORD_II 0x0c > +#define IOP_XTAL32K_PASSWORD_I 0x10 > +#define IOP_XTAL32K_PASSWORD_II 0x14 > +#define IOP_CLK27M_PASSWORD_I 0x18 > +#define IOP_CLK27M_PASSWORD_II 0x1c > +#define IOP_PMC_TIMER2 0x20 > + > +/* Max size of poweroff.bin that can be received */ > +#define POWEROFF_CODE_MAX_SIZE 0x4000 > + > +/* 8051 informs linux kerenl. 8051 has been switched to poweroff.bin code. */ > +#define IOP_READY 0x0004 > +#define RISC_READY 0x0008 > + > +/* System linux kernel tells 8051 which gpio pin to wake-up through. */ > +#define WAKEUP_PIN 0xFE02 > + > +/* > + * There are 3 power domains in SP7021, AO domain, IOP domain, > + * Default domain. Default domain is linux kernel system. > + * System linux kernel tells 8051 to execute power off. > + */ > +#define DEFAULT_DOMAIN_POWEROFF 0x5331 /* AO&IOP domain on, Default domain off */ > +#define DEFAULT_AND_IOP_DOMAIN_POWEROFF 0x5333 /* AO domain on, IOP&Default domain off */ > + > +struct sp_iop { > + struct device *dev; > + struct clk *iopclk; > + struct reset_control *rstc; > + void __iomem *iop_regs; > + void __iomem *pmc_regs; > + void __iomem *moon0_regs; > + int irq; > + int gpio_wakeup; > + resource_size_t iop_mem_start; > + resource_size_t iop_mem_size; > + unsigned char bin_code_mode; > +}; > + > +static struct sp_iop *iop_poweroff; > + > +static void sp_iop_load_normal_code(struct sp_iop *iop) > +{ > + const struct firmware *fw; > + void __iomem *iop_kernel_base; > + unsigned int reg, err; > + > + err = request_firmware(&fw, "normal.bin", iop->dev); > + if (err) > + dev_err(iop->dev, "get bin file error\n"); > + > + iop_kernel_base = ioremap(iop->iop_mem_start, fw->size); > + memset(iop_kernel_base, 0, fw->size); > + memcpy(iop_kernel_base, fw->data, fw->size); > + release_firmware(fw); > + > + clk_prepare_enable(iop->iopclk); where do you disable the clock? > + > + reg = readl(iop->iop_regs + IOP_CONTROL); > + reg |= 0x01; > + writel(reg, iop->iop_regs + IOP_CONTROL); > + (...) > +static int sp_iop_get_resources(struct platform_device *pdev, struct sp_iop *p_sp_iop_info) > +{ > + struct resource *r; > + > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop"); > + p_sp_iop_info->iop_regs = devm_ioremap_resource(&pdev->dev, r); > + if (IS_ERR(p_sp_iop_info->iop_regs)) { > + dev_err(&pdev->dev, "ioremap fail\n"); > + return PTR_ERR(p_sp_iop_info->iop_regs); > + } > + > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop_pmc"); > + p_sp_iop_info->pmc_regs = devm_ioremap_resource(&pdev->dev, r); > + if (IS_ERR(p_sp_iop_info->pmc_regs)) { > + dev_err(&pdev->dev, "ioremap fail\n"); > + return PTR_ERR(p_sp_iop_info->pmc_regs); > + } > + > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon0"); > + p_sp_iop_info->moon0_regs = devm_ioremap_resource(&pdev->dev, r); > + if (IS_ERR(p_sp_iop_info->moon0_regs)) { > + dev_err(&pdev->dev, "ioremap fail\n"); > + return PTR_ERR(p_sp_iop_info->moon0_regs); > + } > + return 0; https://lore.kernel.org/all/16d98dfa-66f9-f9a4-08c9-d68d78c33b23@xxxxxxxxxxxxx/ You received a comment about adding blank lines before return. This has to be done everywhere. > +} > + > +static int sp_iop_platform_driver_probe(struct platform_device *pdev) Do not add "platform_driver" suffix to functions. "_probe" is enough. > +{ > + int ret = -ENXIO; > + int rc; > + struct sp_iop *iop; > + struct device_node *memnp; > + struct resource mem_res; > + > + iop = devm_kzalloc(&pdev->dev, sizeof(struct sp_iop), GFP_KERNEL); > + if (!iop) { > + ret = -ENOMEM; > + goto fail_kmalloc; > + } > + > + ret = sp_iop_get_resources(pdev, iop); > + > + /* Get reserve address. */ > + memnp = of_parse_phandle(pdev->dev.of_node, "memory-region", 0); > + if (!memnp) { > + dev_err(&pdev->dev, "no memory-region node\n"); > + return -EINVAL; > + } > + > + rc = of_address_to_resource(memnp, 0, &mem_res); > + of_node_put(memnp); > + if (rc) { > + dev_err(&pdev->dev, "failed to translate memory-region to a resource\n"); > + return -EINVAL; > + } > + > + iop->dev = &pdev->dev; > + iop->iop_mem_start = mem_res.start; > + iop->iop_mem_size = resource_size(&mem_res); > + iop->iopclk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(iop->iopclk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(iop->iopclk), > + "devm_clk_get fail\n"); Clocks are not documented but required in bindings? > + > + /* > + * 8051 has two bin files, normal.bin and poweroff.bin in rootfs. > + * Normally, 8051 executes normal.bin code. Normal.bin code size can exceed 16K. > + */ > + sp_iop_load_normal_code(iop); > + > + platform_set_drvdata(pdev, iop); > + iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node, "iop-wakeup", 0); It's not in the bindings. Do not add undocumented properties. Everything has to be in the bindings. I don't see usage of generic wakeup-gpios, so this all looks untested. You didn't run this code, did you? > + iop_poweroff = iop; > + pm_power_off = sp_iop_poweroff; > + > + return 0; > + > +fail_kmalloc: > + return ret; > +} > + > +static int sp_iop_remove(struct platform_device *pdev) > +{ > + pm_power_off = NULL; > + return 0; > +} > + > +static const struct of_device_id sp_iop_of_match[] = { > + { .compatible = "sunplus,sp7021-iop" }, > + { /* sentinel */ }, > +}; > + > +MODULE_DEVICE_TABLE(of, sp_iop_of_match); > + > +static struct platform_driver sp_iop_platform_driver = { > + .probe = sp_iop_platform_driver_probe, > + .remove = sp_iop_remove, > + .driver = { > + .name = "sunplus,sp7021-iop", > + .of_match_table = sp_iop_of_match, > + } > +}; > + > +module_platform_driver(sp_iop_platform_driver); > + > +MODULE_AUTHOR("Tony Huang <tonyhuang.sunplus@xxxxxxxxx>"); > +MODULE_DESCRIPTION("Sunplus IOP Driver"); > +MODULE_LICENSE("GPL v2"); Best regards, Krzysztof
