| Message ID | cover.1638499659.git.tonyhuang.sunplus@gmail.com |
|---|---|
| Headers | show |
| Series | Add iop driver for Sunplus SP7021 | expand |
On Fri, Dec 03, 2021 at 11:48:45AM +0800, Tony Huang wrote: > +#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]; Please make your local variables static so that yhou do not polute the kernel's global symbol table. > +/* ---------------------------------------------------------------------------------------------- */ > +resource_size_t SP_IOP_RESERVE_BASE; > +resource_size_t SP_IOP_RESERVE_SIZE; > +/* ---------------------------------------------------------------------------------------------- */ Again, static. And why the odd comment lines? And those are not good variable names. > +struct sp_iop { > + struct miscdevice dev; // iop device > + struct mutex write_lock; > + void __iomem *iop_regs; > + void __iomem *pmc_regs; > + void __iomem *moon0_regs; > + int irq; > +}; > +/***************************************************************** > + * G L O B A L D A T A > + ******************************************************************/ Global where? What about the ones above? :) > +static struct sp_iop *iop; Why do you think you only have one device in the system? Please do not use a single variable like this. It is easy to make your driver handle an unlimited number of devices just as easy as to handle 1 device. Please do that instead and hang your device-specific data off of the correct data structures that the driver core gives you. > + > +void iop_normal_mode(void) Did you run sparse on this code? Please do so. Also, no need for a .h file for a driver that only has one .c file. thanks, greg k-h
On Fri, Dec 03, 2021 at 11:48:45AM +0800, Tony Huang 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@gmail.com> > --- > Changes in v2: > - Addressed comments from Arnd Bergmann. > - Addressed comments from Greg KH. > - Addressed comments from kernel test robot. > > MAINTAINERS | 1 + > drivers/misc/Kconfig | 1 + > drivers/misc/Makefile | 1 + > drivers/misc/iop/Kconfig | 13 ++ > drivers/misc/iop/Makefile | 6 + Why do you need a subdirectory for a single .c file? thanks, greg k-h
On Fri, Dec 03, 2021 at 11:48:45AM +0800, Tony Huang wrote: > --- /dev/null > +++ b/drivers/misc/iop/sunplus_iop.c > @@ -0,0 +1,518 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later Are you sure about "or later"? I have to ask. Also, no copyright information on the file? Are you sure about that? thanks, greg k-h
On Fri, Dec 3, 2021 at 4:48 AM Tony Huang <tonyhuang.sunplus@gmail.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@gmail.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. > +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. > +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. > +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. > +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. > 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 The rest of the header only describes the hardware itself, so I'd suggest merging all of it into the .c file. Arnd
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@gmail.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@gmail.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.
Dear Greg KH: > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Friday, December 3, 2021 6:39 PM > To: Tony Huang <tonyhuang.sunplus@gmail.com> > Cc: derek.kiernan@xilinx.com; dragan.cvetic@xilinx.com; arnd@arndb.de; > linux-kernel@vger.kernel.org; robh+dt@kernel.org; devicetree@vger.kernel.org; > Wells Lu 呂芳騰 <wells.lu@sunplus.com>; Tony Huang 黃懷厚 > <tony.huang@sunplus.com> > Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 > > On Fri, Dec 03, 2021 at 11:48:45AM +0800, Tony Huang wrote: > > +#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]; > > Please make your local variables static so that yhou do not polute the kernel's > global symbol table. > I will modify it. > > +/* > > +--------------------------------------------------------------------- > > +------------------------- */ resource_size_t SP_IOP_RESERVE_BASE; > > +resource_size_t SP_IOP_RESERVE_SIZE; > > +/* > > +--------------------------------------------------------------------- > > +------------------------- */ > > Again, static. > > And why the odd comment lines? > I will remove it. > And those are not good variable names. > > > +struct sp_iop { > > + struct miscdevice dev; // iop device > > + struct mutex write_lock; > > + void __iomem *iop_regs; > > + void __iomem *pmc_regs; > > + void __iomem *moon0_regs; > > + int irq; > > +}; > > > +/************************************************************** > *** > > + * G L O B A L D A T A > > + > **************************************************************** > **/ > > Global where? What about the ones above? :) > I will remove it. > > +static struct sp_iop *iop; > > Why do you think you only have one device in the system? Please do not use > a single variable like this. It is easy to make your driver handle an unlimited > number of devices just as easy as to handle 1 device. > Please do that instead and hang your device-specific data off of the correct > data structures that the driver core gives you. > I will modify it. > > + > > +void iop_normal_mode(void) > > Did you run sparse on this code? Please do so. > > Also, no need for a .h file for a driver that only has one .c file. > I need to keep sunplus_iop.h. Other files will use sp_iop_platform_driver_poweroff(void) in poweroff flow. > thanks, > > greg k-h
Dear Greg KH, Arnd: > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Friday, December 3, 2021 6:39 PM > To: Tony Huang <tonyhuang.sunplus@gmail.com> > Cc: derek.kiernan@xilinx.com; dragan.cvetic@xilinx.com; arnd@arndb.de; > linux-kernel@vger.kernel.org; robh+dt@kernel.org; devicetree@vger.kernel.org; > Wells Lu 呂芳騰 <wells.lu@sunplus.com>; Tony Huang 黃懷厚 > <tony.huang@sunplus.com> > Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 > > On Fri, Dec 03, 2021 at 11:48:45AM +0800, Tony Huang 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@gmail.com> > > --- > > Changes in v2: > > - Addressed comments from Arnd Bergmann. > > - Addressed comments from Greg KH. > > - Addressed comments from kernel test robot. > > > > MAINTAINERS | 1 + > > drivers/misc/Kconfig | 1 + > > drivers/misc/Makefile | 1 + > > drivers/misc/iop/Kconfig | 13 ++ > > drivers/misc/iop/Makefile | 6 + > > Why do you need a subdirectory for a single .c file? > 1. Currently my bin file is placed in the root file system. I need to wait for the kernel booting to succeed before loading the bin code. 2. In addition, I need the kernel booting process to be able to mount the iop module and load bin file.I need to put bin file in /iop. Do you have a good way to load bin code during kernel booting? Thanks
On Mon, Dec 06, 2021 at 03:50:03AM +0000, Tony Huang 黃懷厚 wrote: > > Also, no need for a .h file for a driver that only has one .c file. > > > > I need to keep sunglus_iop.h. Other files will use > sp_iop_platform_driver_poweroff(void) in poweroff flow. What other files? That is not included here, nor should other drivers be making that call, use the normal poweroff logic. thanks, greg k-h
On Mon, Dec 6, 2021 at 4:42 AM Tony Huang 黃懷厚 <tony.huang@sunplus.com> wrote: > > 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@gmail.com> wrote: > > > + > > > +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. Why is this a useful interface to have? If this is only for debugging, a tracepoint may be more useful than a full character device. > > 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. Ok, in that case you can probably just remove the empty callback functions. Arnd
On Mon, Dec 06, 2021 at 06:48:46AM +0000, Tony Huang 黃懷厚 wrote: > Dear Greg KH, Arnd: > > > -----Original Message----- > > From: Greg KH <gregkh@linuxfoundation.org> > > Sent: Friday, December 3, 2021 6:39 PM > > To: Tony Huang <tonyhuang.sunplus@gmail.com> > > Cc: derek.kiernan@xilinx.com; dragan.cvetic@xilinx.com; arnd@arndb.de; > > linux-kernel@vger.kernel.org; robh+dt@kernel.org; devicetree@vger.kernel.org; > > Wells Lu 呂芳騰 <wells.lu@sunplus.com>; Tony Huang 黃懷厚 > > <tony.huang@sunplus.com> > > Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 > > > > On Fri, Dec 03, 2021 at 11:48:45AM +0800, Tony Huang 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@gmail.com> > > > --- > > > Changes in v2: > > > - Addressed comments from Arnd Bergmann. > > > - Addressed comments from Greg KH. > > > - Addressed comments from kernel test robot. > > > > > > MAINTAINERS | 1 + > > > drivers/misc/Kconfig | 1 + > > > drivers/misc/Makefile | 1 + > > > drivers/misc/iop/Kconfig | 13 ++ > > > drivers/misc/iop/Makefile | 6 + > > > > Why do you need a subdirectory for a single .c file? > > > > 1. Currently my bin file is placed in the root file system. I need to > wait for the kernel booting to succeed before loading the bin code. What "bin file"? > 2. In addition, I need the kernel booting process to be able to mount > the iop module and load bin file.I need to put bin file in /iop. That is an odd directory location. > Do you have a good way to load bin code during kernel booting? Is this firmware? Put it in the normal location for firware that the kernel expects to see. thanks, greg k-h
Dear Greg KH: > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Monday, December 6, 2021 4:07 PM > To: Tony Huang 黃懷厚 <tony.huang@sunplus.com> > Cc: Tony Huang <tonyhuang.sunplus@gmail.com>; derek.kiernan@xilinx.com; > dragan.cvetic@xilinx.com; arnd@arndb.de; linux-kernel@vger.kernel.org; > robh+dt@kernel.org; devicetree@vger.kernel.org; Wells Lu 呂芳騰 > <wells.lu@sunplus.com> > Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 > > On Mon, Dec 06, 2021 at 06:48:46AM +0000, Tony Huang 黃懷厚 wrote: > > Dear Greg KH, Arnd: > > > > > -----Original Message----- > > > From: Greg KH <gregkh@linuxfoundation.org> > > > Sent: Friday, December 3, 2021 6:39 PM > > > To: Tony Huang <tonyhuang.sunplus@gmail.com> > > > Cc: derek.kiernan@xilinx.com; dragan.cvetic@xilinx.com; > > > arnd@arndb.de; linux-kernel@vger.kernel.org; robh+dt@kernel.org; > > > devicetree@vger.kernel.org; Wells Lu 呂芳騰 <wells.lu@sunplus.com>; > > > Tony Huang 黃懷厚 > > > <tony.huang@sunplus.com> > > > Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 > > > > > > On Fri, Dec 03, 2021 at 11:48:45AM +0800, Tony Huang 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@gmail.com> > > > > --- > > > > Changes in v2: > > > > - Addressed comments from Arnd Bergmann. > > > > - Addressed comments from Greg KH. > > > > - Addressed comments from kernel test robot. > > > > > > > > MAINTAINERS | 1 + > > > > drivers/misc/Kconfig | 1 + > > > > drivers/misc/Makefile | 1 + > > > > drivers/misc/iop/Kconfig | 13 ++ > > > > drivers/misc/iop/Makefile | 6 + > > > > > > Why do you need a subdirectory for a single .c file? > > > > > > > 1. Currently my bin file is placed in the root file system. I need to > > wait for the kernel booting to succeed before loading the bin code. > > What "bin file"? > 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. > > 2. In addition, I need the kernel booting process to be able to mount > > the iop module and load bin file.I need to put bin file in /iop. > > That is an odd directory location. > > > Do you have a good way to load bin code during kernel booting? > > Is this firmware? Put it in the normal location for firware that the kernel > expects to see. > > thanks, > > greg k-h
Dear Arnd: > -----Original Message----- > From: Arnd Bergmann <arnd@arndb.de> > Sent: Monday, December 6, 2021 4:04 PM > To: Tony Huang 黃懷厚 <tony.huang@sunplus.com> > Cc: Arnd Bergmann <arnd@arndb.de>; Tony Huang > <tonyhuang.sunplus@gmail.com>; Derek Kiernan <derek.kiernan@xilinx.com>; > Dragan Cvetic <dragan.cvetic@xilinx.com>; gregkh > <gregkh@linuxfoundation.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; Rob Herring <robh+dt@kernel.org>; DTML > <devicetree@vger.kernel.org>; Wells Lu 呂芳騰 <wells.lu@sunplus.com> > Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 > > On Mon, Dec 6, 2021 at 4:42 AM Tony Huang 黃懷厚 > <tony.huang@sunplus.com> wrote: > > > 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@gmail.com> wrote: > > > > + > > > > +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. > > Why is this a useful interface to have? If this is only for debugging, a tracepoint > may be more useful than a full character device. > Yes, it is for debugging. > > > 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. > > Ok, in that case you can probably just remove the empty callback functions. > > Arnd
On Mon, Dec 06, 2021 at 08:22:01AM +0000, Tony Huang 黃懷厚 wrote: > Dear Greg KH: > > > -----Original Message----- > > From: Greg KH <gregkh@linuxfoundation.org> > > Sent: Monday, December 6, 2021 4:07 PM > > To: Tony Huang 黃懷厚 <tony.huang@sunplus.com> > > Cc: Tony Huang <tonyhuang.sunplus@gmail.com>; derek.kiernan@xilinx.com; > > dragan.cvetic@xilinx.com; arnd@arndb.de; linux-kernel@vger.kernel.org; > > robh+dt@kernel.org; devicetree@vger.kernel.org; Wells Lu 呂芳騰 > > <wells.lu@sunplus.com> > > Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 > > > > On Mon, Dec 06, 2021 at 06:48:46AM +0000, Tony Huang 黃懷厚 wrote: > > > Dear Greg KH, Arnd: > > > > > > > -----Original Message----- > > > > From: Greg KH <gregkh@linuxfoundation.org> > > > > Sent: Friday, December 3, 2021 6:39 PM > > > > To: Tony Huang <tonyhuang.sunplus@gmail.com> > > > > Cc: derek.kiernan@xilinx.com; dragan.cvetic@xilinx.com; > > > > arnd@arndb.de; linux-kernel@vger.kernel.org; robh+dt@kernel.org; > > > > devicetree@vger.kernel.org; Wells Lu 呂芳騰 <wells.lu@sunplus.com>; > > > > Tony Huang 黃懷厚 > > > > <tony.huang@sunplus.com> > > > > Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 > > > > > > > > On Fri, Dec 03, 2021 at 11:48:45AM +0800, Tony Huang 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@gmail.com> > > > > > --- > > > > > Changes in v2: > > > > > - Addressed comments from Arnd Bergmann. > > > > > - Addressed comments from Greg KH. > > > > > - Addressed comments from kernel test robot. > > > > > > > > > > MAINTAINERS | 1 + > > > > > drivers/misc/Kconfig | 1 + > > > > > drivers/misc/Makefile | 1 + > > > > > drivers/misc/iop/Kconfig | 13 ++ > > > > > drivers/misc/iop/Makefile | 6 + > > > > > > > > Why do you need a subdirectory for a single .c file? > > > > > > > > > > 1. Currently my bin file is placed in the root file system. I need to > > > wait for the kernel booting to succeed before loading the bin code. > > > > What "bin file"? > > > > 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. So this is firmware, just put it in the normal firmware location. thanks, greg k-h
Dear Greg KH: > > > Also, no need for a .h file for a driver that only has one .c file. > > > > > > > I need to keep sunglus_iop.h. Other files will use > > sp_iop_platform_driver_poweroff(void) in poweroff flow. > > What other files? That is not included here, nor should other drivers be > making that call, use the normal poweroff logic. > Okay, I will call the sp_iop_platform_driver_poweroff(void) through normal poweroff logic.