Dear Krzysztof: > >> On 12/03/2022 17:16, Tony Huang wrote: > >>> This driver is load 8051 bin code. > >>> The IOP core is DQ8051, so also named IOP8051. > >>> Need Install DQ8051, The DQ8051 bin file generated by keil C. > >>> We will provide users with 8051 normal and power off source code. > >>> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/ > >>> > >> > How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source > >> How+to+use+I+O+processor+8051+of+- > >>> files-for-IOP > >>> Users can follow the operation steps to generate normal.bin and > >>> poweroff.bin. > >>> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338 > >>> /26.+IOP8051 26.5?How To Create 8051 bin file > >>> > >>> PMC in power management purpose: > >>> Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff. > >>> When the power off command is executed. > >>> The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and > >>> power-off of the system. > >>> > >>> Signed-off-by: Tony Huang <tonyhuang.sunplus@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? > >> > > > > drivers/clk : SP7021 system has clock device driver (clk-sp7021.c). > > So I set the IOP clock through the following function. > > clk_prepare_enable(iop->iopclk); > > clk_disable_unprepare(iop->iopclk); > > > > drivers/power/reset : SP7021 system does not have a power off device driver. > > What does it mean? The feedback was to split clk and reset features to > separate drivers and I still see only two patches here with a misc driver. So how > is his comments addressed? You did not reply in that thread. > I finished replying to Arnd. > > > >>> - Addressed comments from krzysztof. > >>> > >>> MAINTAINERS | 1 + > >>> drivers/misc/Kconfig | 23 +++ > >>> drivers/misc/Makefile | 1 + > >>> drivers/misc/sunplus_iop.c | 411 > >>> +++++++++++++++++++++++++++++++++++++++++++++ > >> > >> The driver looks like SoC specific driver. Why did you put it in > >> drivers/misc/, not in usual place - drivers/soc/? > > > > 8051 is designed for processing I/O events, like receiving IR signal from > remote controller, > > taking care of power on or off requests from RTC, or other hardware events > of external peripherals > > even when power of main system is off. > > So I put it in drivers/misc. > > Is IOP8061 a separate device? Your datasheet is saying its embedded in > SP7021 SoC, so it is a soc driver. This does not fit misc driver description (a > "strange device") but a SoC driver description. > IOP is a separate device. CPU is 8051. SP7021 contains three kinds of CPU. Quad-core ARM Cortex-A7 (CA7) ARM926 real-time core 8051 low-power core > > > >> sp_iop_poweroff is still here. > > > > sp_iop_poweroff(): SP7021 system does not have a power off device driver. > > I have to put it here. > > This should be rather in a reset driver, not in a misc one. What is your plan for > this driver? You described the hardware and judging by it, there will be quite a > lot of separate features so it's reasonable to split the driver into separate > logical elements. However keeping all in the same place would be ok, if you do > not plan to add any more features. > This would mean the driver will handle *only* reset and FW loading. > Can I put sp_iop_poweroff() here for now? When power off device driver is added in /driver/power/reset is complete, we will move to it. > > > >>> 4 files changed, 436 insertions(+) > >>> create mode 100644 drivers/misc/sunplus_iop.c > >>> > >>> diff --git a/MAINTAINERS b/MAINTAINERS index d64c8ed..c282e95 100644 > >>> --- a/MAINTAINERS > >>> +++ b/MAINTAINERS > >>> @@ -18246,6 +18246,7 @@ SUNPLUS IOP DRIVER > >>> M: Tony Huang <tonyhuang.sunplus@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 > >> How+to+use+I+O+processor+8051+of+- > >> files-for-IOP > >>> + Users can follow the operation steps to generate normal.bin and > >> poweroff.bin. > >>> + Path: > >> https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP > >> 8051 > >>> + 26.5?How To Create 8051 bin file > >>> + > >>> + PMC in power management purpose: > >>> + Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff. > >>> + When the power off command is executed. > >>> + The 8051 has a register(DEF_PWR_EN_0=0) to control the > power-on > >> and > >>> + power-off of the system. > >>> + > >>> + This driver can also be built as a module. If so, the module > >>> + will be called sunplus_iop. > >>> + > >>> source "drivers/misc/c2port/Kconfig" > >>> source "drivers/misc/eeprom/Kconfig" > >>> source "drivers/misc/cb710/Kconfig" > >>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index > >>> a086197..eafeab6 100644 > >>> --- a/drivers/misc/Makefile > >>> +++ b/drivers/misc/Makefile > >>> @@ -52,6 +52,7 @@ obj-$(CONFIG_DW_XDATA_PCIE) += dw-xdata-pcie.o > >>> obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o > >>> obj-$(CONFIG_OCXL) += ocxl/ > >>> obj-$(CONFIG_BCM_VK) += bcm-vk/ > >>> +obj-$(CONFIG_SUNPLUS_IOP) += sunplus_iop.o > >>> obj-y += cardreader/ > >>> obj-$(CONFIG_PVPANIC) += pvpanic/ > >>> obj-$(CONFIG_HABANA_AI) += habanalabs/ > >>> diff --git a/drivers/misc/sunplus_iop.c b/drivers/misc/sunplus_iop.c > >>> new file mode 100644 index 0000000..5bdce5e > >>> --- /dev/null > >>> +++ b/drivers/misc/sunplus_iop.c > >>> @@ -0,0 +1,411 @@ > >>> +// SPDX-License-Identifier: GPL-2.0 > >>> +/* > >>> + * The IOP driver for Sunplus SP7021 > >>> + * > >>> + * Copyright (C) 2021 Sunplus Technology Inc. > >>> + * > >>> + * All Rights Reserved. > >>> + */ > >>> +#include <linux/clk.h> > >>> +#include <linux/delay.h> > >>> +#include <linux/dma-mapping.h> > >>> +#include <linux/firmware.h> > >>> +#include <linux/iopoll.h> > >>> +#include <linux/module.h> > >>> +#include <linux/of_platform.h> > >>> +#include <linux/of_address.h> > >>> +#include <linux/of_gpio.h> > >>> + > >>> +/* IOP register offset */ > >>> +#define IOP_CONTROL 0x00 > >>> +#define IOP_DATA0 0x20 > >>> +#define IOP_DATA1 0x24 > >>> +#define IOP_DATA2 0x28 > >>> +#define IOP_DATA3 0x2c > >>> +#define IOP_DATA4 0x30 > >>> +#define IOP_DATA5 0x34 > >>> +#define IOP_DATA6 0x38 > >>> +#define IOP_DATA7 0x3c > >>> +#define IOP_DATA8 0x40 > >>> +#define IOP_DATA9 0x44 > >>> +#define IOP_DATA10 0x48 > >>> +#define IOP_DATA11 0x4c > >>> +#define IOP_BASE_ADR_L 0x50 > >>> +#define IOP_BASE_ADR_H 0x54 > >>> + > >>> +/* PMC register offset */ > >>> +#define IOP_PMC_TIMER 0x00 > >>> +#define IOP_PMC_CTRL 0x04 > >>> +#define IOP_XTAL27M_PASSWORD_I 0x08 > >>> +#define IOP_XTAL27M_PASSWORD_II 0x0c > >>> +#define IOP_XTAL32K_PASSWORD_I 0x10 > >>> +#define IOP_XTAL32K_PASSWORD_II 0x14 > >>> +#define IOP_CLK27M_PASSWORD_I 0x18 > >>> +#define IOP_CLK27M_PASSWORD_II 0x1c > >>> +#define IOP_PMC_TIMER2 0x20 > >>> + > >>> +/* Max size of poweroff.bin that can be received */ #define > >>> +POWEROFF_CODE_MAX_SIZE 0x4000 > >>> + > >>> +/* 8051 informs linux kerenl. 8051 has been switched to poweroff.bin > code. > >> */ > >>> +#define IOP_READY 0x0004 > >>> +#define RISC_READY 0x0008 > >>> + > >>> +/* System linux kernel tells 8051 which gpio pin to wake-up through. */ > >>> +#define WAKEUP_PIN 0xFE02 > >>> + > >>> +/* > >>> + * There are 3 power domains in SP7021, AO domain, IOP domain, > >>> + * Default domain. Default domain is linux kernel system. > >>> + * System linux kernel tells 8051 to execute power off. > >>> + */ > >>> +#define DEFAULT_DOMAIN_POWEROFF 0x5331 /* AO&IOP domain > on, > >> Default domain off */ > >>> +#define DEFAULT_AND_IOP_DOMAIN_POWEROFF 0x5333 /* AO > domain > >> on, IOP&Default domain off */ > >>> + > >>> +struct sp_iop { > >>> + struct device *dev; > >>> + struct clk *iopclk; > >>> + struct reset_control *rstc; > >>> + void __iomem *iop_regs; > >>> + void __iomem *pmc_regs; > >>> + void __iomem *moon0_regs; > >>> + int irq; > >>> + int gpio_wakeup; > >>> + resource_size_t iop_mem_start; > >>> + resource_size_t iop_mem_size; > >>> + unsigned char bin_code_mode; > >>> +}; > >>> + > >>> +static struct sp_iop *iop_poweroff; > >>> + > >>> +static void sp_iop_load_normal_code(struct sp_iop *iop) { > >>> + const struct firmware *fw; > >>> + void __iomem *iop_kernel_base; > >>> + unsigned int reg, err; > >>> + > >>> + err = request_firmware(&fw, "normal.bin", iop->dev); > >>> + if (err) > >>> + dev_err(iop->dev, "get bin file error\n"); > >>> + > >>> + iop_kernel_base = ioremap(iop->iop_mem_start, fw->size); > >>> + memset(iop_kernel_base, 0, fw->size); > >>> + memcpy(iop_kernel_base, fw->data, fw->size); > >>> + release_firmware(fw); > >>> + > >>> + clk_prepare_enable(iop->iopclk); > >> > >> where do you disable the clock? > > > > I will add disable clock in sp_iop_remove(). > > Below is my modification > > static int sp_iop_remove(struct platform_device *pdev) > > { > > struct sp_iop *iop = iop_poweroff; > > pm_power_off = NULL; > > clk_disable_unprepare(iop->iopclk); > > > > return 0; > > } > > How is it balanced then? remove() is symmetric to probe() but you did not > enable clocks in the probe(). Instead you enable the clocks each time in > load_normal_code() and load_poweroff_code(). > > This does not seem right at all. OK, I will modify it.
