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

From: Tony Huang 黃懷厚
Date: Mon Mar 14 2022 - 22:08:21 EST

Next message: Stephen Rothwell: "linux-next: manual merge of the tip tree with the powerpc tree"
Previous message: Alexander Vorwerk: "[PATCH] tty: serial: jsm: fix two assignments in if conditions"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

Next message: Stephen Rothwell: "linux-next: manual merge of the tip tree with the powerpc tree"
Previous message: Alexander Vorwerk: "[PATCH] tty: serial: jsm: fix two assignments in if conditions"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]