Message-ID: <58fd70a3fa3d44229edd849cab49eadf@sphcmbx02.sunplus.com.tw>
Date:   Tue, 15 Mar 2022 02:08:20 +0000
From:   Tony Huang 黃懷厚 <tony.huang@...plus.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>,
        Tony Huang <tonyhuang.sunplus@...il.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "derek.kiernan@...inx.com" <derek.kiernan@...inx.com>,
        "dragan.cvetic@...inx.com" <dragan.cvetic@...inx.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     Wells Lu 呂芳騰 <wells.lu@...plus.com>
Subject: RE: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021

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@...il.com>
> >>> ---
> >>> 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@...il.com>
> >>>  S:	Maintained
> >>>  F:	Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> >>> +F:	drivers/misc/sunplus_iop.c
> >>>
> >>>  SUPERH
> >>>  M:	Yoshinori Sato <ysato@...rs.sourceforge.jp>
> >>> 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.

Powered by blists - more mailing lists