Message-ID: <CAK8P3a2TmizZhkqxqB9DnmjnwzRZwWH+90Wc7WWaCMCm7KP7pg@mail.gmail.com>
Date:   Mon, 7 Mar 2022 14:42:49 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Tony Huang <tonyhuang.sunplus@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        DTML <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Derek Kiernan <derek.kiernan@...inx.com>,
        Dragan Cvetic <Dragan.cvetic@...inx.com>,
        Arnd Bergmann <arnd@...db.de>,
        gregkh <gregkh@...uxfoundation.org>,
        Wells Lu 呂芳騰 <wells.lu@...plus.com>,
        Tony Huang <tony.huang@...plus.com>
Subject: Re: [PATCH v10 2/2] misc: Add iop driver for Sunplus SP7021

On Mon, Mar 7, 2022 at 6:25 AM Tony Huang <tonyhuang.sunplus@...il.com> 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.
> Changes in v10:
>  - Added sp_iop_poweroff function for poweroff command.
>

Thank you for finally adding support for one of the functions of the
hardware!

> 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 description sounds misleading here: At the moment, you only add
support for poweroff, not for system suspend.

Maybe leave out the description about the RTC and power savings here
and only describe the bits that the driver actually implements. Can you
add some text to the patch changelog to describe what your plans are
for supporting suspend mode, and clarify which functions are implemented
already, compared to those that are possible in hardware but not part
of this patch series?


> +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);

'standby' mode usually refers to 'suspend-to-ram' mode, which is something
the driver does not (yet) support. Can you clarify whether that means you
want to add it later, or you just used the wrong term here?

> +
> +/* 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

Again, the names here seem misleading: in power management terms,
's1' and 's3' typically refer to types of system power saving modes that
are different from power-off or suspend-to-disk. Maybe try to use less
confusing terms here?

> +       /* 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));

This looks like you are writing individual bits into a shared
clock/reset controller that is part of the SoC. If this is the case,
it would be better to make that a separate driver that owns
the moon0_regs registers and exposes them through the
clk and reset subsystem interfaces (drivers/clk, drivers/reset).

> +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);
> +}

The power-off logic should probably be a separate driver in drivers/power/reset/
that calls into the common driver.

        Arnd

Powered by blists - more mailing lists