Message-ID: <9233c42fc4714122a3054a6c5bda0d3e@sphcmbx02.sunplus.com.tw>
Date:   Mon, 6 Dec 2021 03:42:02 +0000
From:   Tony Huang 黃懷厚 <tony.huang@...plus.com>
To:     Arnd Bergmann <arnd@...db.de>,
        Tony Huang <tonyhuang.sunplus@...il.com>
CC:     Derek Kiernan <derek.kiernan@...inx.com>,
        Dragan Cvetic <dragan.cvetic@...inx.com>,
        gregkh <gregkh@...uxfoundation.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        DTML <devicetree@...r.kernel.org>,
        Wells Lu 呂芳騰 <wells.lu@...plus.com>
Subject: RE: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

Dear Arnd:

> Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021
> 
> On Fri, Dec 3, 2021 at 4:48 AM Tony Huang <tonyhuang.sunplus@...il.com>
> wrote:
> >
> > IOP (IO Processor) embedded inside SP7021 which is used as Processor
> > for I/O control, RTC wake-up and cooperation with CPU & PMC in power
> > management purpose.
> > The IOP core is DQ8051, so also named IOP8051, it supports dedicated
> > JTAG debug pins which share with SP7021.
> > In standby mode operation, the power spec reach 400uA.
> >
> > Signed-off-by: Tony Huang <tonyhuang.sunplus@...il.com>
> > ---
> > Changes in v2:
> >  - Addressed comments from Arnd Bergmann.
> >  - Addressed comments from Greg KH.
> >  - Addressed comments from kernel test robot.
> 
> This looks much better already.
> 
> > +#define NORMAL_CODE_MAX_SIZE 0X1000
> > +#define STANDBY_CODE_MAX_SIZE 0x4000
> > +unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE];
> > +unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE];
> 
> I think these should be part of the sp_iop structure, not global variables.
> 

I will modify it.

> > +static struct sp_iop *iop;
> > +
> > +void iop_normal_mode(void)
> > +{
> > +       struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> 
> local functions should generally be 'static' and take the 'struct sp_iop' instance
> pointer as the first argument.
> 
> Functions that need to be global because they are used by other drivers (if any)
> should probably also be exported and have documentation above their
> definition.
> 

I will mocify it.

> > +static int  get_normal_code(struct device *dev) {
> > +       const struct firmware *fw;
> > +       static const char file[] = "normal.bin";
> > +       unsigned int err, i;
> > +
> > +       dev_info(dev, "normal code\n");
> > +       err = request_firmware(&fw, file, dev);
> 
> The file name needs to clearly identify the device to avoid conflicts with other
> drivers.
> 

I will moidfy it.

> > +static int sp_iop_open(struct inode *inode, struct file *pfile) {
> > +       return 0;
> > +}
> > +
> > +static ssize_t sp_iop_read(struct file *pfile, char __user *ubuf,
> > +                       size_t length, loff_t *offset) {
> > +       return 0;
> > +}
> > +
> > +static ssize_t sp_iop_write(struct file *pfile, const char __user *ubuf,
> > +       size_t length, loff_t *offset) {
> > +       return 0;
> > +}
> > +
> > +static int sp_iop_release(struct inode *inode, struct file *pfile) {
> > +       //dev_dbg(iop->dev, "Sunplus IOP module release\n");
> > +       return 0;
> > +}
> > +
> > +static const struct file_operations sp_iop_fops = {
> > +       .owner                  = THIS_MODULE,
> > +       .open                   = sp_iop_open,
> > +       .read                   = sp_iop_read,
> > +       .write                  = sp_iop_write,
> > +       .release                = sp_iop_release,
> > +};
> 
> This does nothing because all the callbacks are empty. You removed the
> inappropriate user space interfaces as I asked you to, but if there is no way for
> either kernel or user space to interact with the hardware, I don't see a point in
> merging the driver until you add a new interface that is usable.
> 

I will modify sp_iop_read() to monitor IOP mailbox data.

> > +static int sp_iop_platform_driver_remove(struct platform_device
> > +*pdev) {
> > +       return 0;
> > +}
> > +
> > +static int sp_iop_platform_driver_suspend(struct platform_device
> > +*pdev, pm_message_t state) {
> > +       return 0;
> > +}
> > +
> > +static void sp_iop_platform_driver_shutdown(struct platform_device
> > +*pdev) {
> > +
> > +}
> > +
> > +void sp_iop_platform_driver_poweroff(void)
> > +{
> > +       iop_standby_mode();
> > +       iop_shutdown();
> > +}
> 
> Something looks wrong here, maybe reread the documentation for runtime
> power management to find a way of putting the device into low-power mode
> when it is unused.
> 

When the poweroff command is executed, the run sp_iop_platform_driver_poweroff(void) function will enter the standby mode. The power off will be executed.
In the system, IOP can continue to work when other modules in the system enter standby / power down modes to monitor whether the system wakes up through RTC.

26.4.2 IOP MODULE EXECUTES 8051 CODE
Source code should reserve SDRAM memory area for IOP module code. 8051 bin file normal code and standby code can be placed in this area. The location area can be select by user.
Normal code: Monitor CPU commands.
Standby code: For RTC wake up, cooperation with CPU&PMC in power management
When the system enters standby mode, 8051 bin file should be moved to I_Cache.
I_Cache has 16K only. Standby code cannot exceed 16K.
When the IOP module is mounted, CPU load 8051 codes (normal.bin) into memory.
Iop_base_addr_l and iop_base_addr_h specify address.
During system boot up, when the IOP is mounted, it will load 8051 normal code and start execute 8051 code.

In the system, IOP can continue to work when other modules in the system enter standby / power down modes to monitor whether the system wakes up through RTC. In such a situation, the IOP will not be able to access the system memory.

> > diff --git a/drivers/misc/iop/sunplus_iop.h
> > b/drivers/misc/iop/sunplus_iop.h new file mode 100644 index
> > 0000000..fcbfd26
> > --- /dev/null
> > +++ b/drivers/misc/iop/sunplus_iop.h
> > @@ -0,0 +1,64 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later*/
> > +
> > +#ifndef __SP_IOP_H__
> > +#define __SP_IOP_H__
> > +#include <mach/io_map.h>
> 
> mach/io_map.h does not exist, so the driver won't compile. I don't think you
> need anything else, so it should be fine to remove the #include
> 

I will remove io_map.h file.

> The rest of the header only describes the hardware itself, so I'd suggest
> merging all of it into the .c file.
>

I need to keep sunplus_iop.h. Other files will use sp_iop_platform_driver_poweroff(void) in poweroff flow.
When the poweroff command is executed, the run sp_iop_platform_driver_poweroff(void) function will enter the standby mode. The power off will be executed.

Powered by blists - more mailing lists