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

From: Tony Huang 黃懷厚
Date: Mon Jan 03 2022 - 04:44:04 EST

Next message: Wolfram Sang: "Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes"
Previous message: Bartosz Golaszewski: "Re: [PATCH] gpio: max3191x: Use bitmap_free() to free bitmap"
In reply to: Greg KH: "Re: [PATCH v6 2/2] misc: Add iop driver for Sunplus SP7021"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Dear Greg:

> > > > IOP(8051) embedded inside SP7021 which is used as Processor for
> > > > I/O control, monitor RTC interrupt 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@xxxxxxxxx>
> > > > ---
> > > > Changes in v6:
> > > > - Added sysfs read/write description.
> > > > - Modify sysfs read function.
> > > > - Addressed comments from kernel test robot.
> > > >
> > > > Documentation/ABI/testing/sysfs-platform-soc@B | 25 ++
> > > > MAINTAINERS | 2 +
> > > > drivers/misc/Kconfig | 12 +
> > > > drivers/misc/Makefile | 1 +
> > > > drivers/misc/sunplus_iop.c | 476
> > > +++++++++++++++++++++++++
> > > > 5 files changed, 516 insertions(+) create mode 100644
> > > > Documentation/ABI/testing/sysfs-platform-soc@B
> > > > create mode 100644 drivers/misc/sunplus_iop.c
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-platform-soc@B
> > > > b/Documentation/ABI/testing/sysfs-platform-soc@B
> > > > new file mode 100644
> > > > index 0000000..6272919
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-platform-soc@B
> > > > @@ -0,0 +1,25 @@
> > > > +What:
> /sys/devices/platform/soc@B/9c000400.iop/sp_iop_mailbox
> > > > +Date: December 2021
> > > > +KernelVersion: 5.16
> > > > +Contact: Tony Huang <tonyhuang.sunplus@xxxxxxxxx>
> > > > +Description:
> > > > + Show 8051 mailbox0 data.
> > >
> > > What format is the data in?
> > >
> >
> > Unsigned short
>
> So you are returning 16 bits, please describe the value as to what it will
> contain and what it means.
>
> And what exactly does "8051" mean here? That is just some random
> processor in the system, it needs to be described better please.
>
> > > > +config SUNPLUS_IOP
> > > > + tristate "Sunplus IOP support"
> > > > + default ARCH_SUNPLUS
> > > > + help
> > > > + Sunplus I/O processor (8051) driver.
> > > > + Processor for I/O control, RTC wake-up proceduce management,
> > > > + and cooperation with CPU&PMC in power management.
> > > > + Need Install DQ8051, The DQ8051 bin file generated by keil C.
> > >
> > > I do not understand this sentence, what do you mean by it? Can you
> > > provide a link to where the files that are required are? Why not
> > > include them in the linux-firmware project?
> > >
> >
> > 1.We will provide users with 8051 normal and standby 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-files-for-IOP
>
> > 2.Users can follow the operation steps to generate normal.bin and
> standby.bin.
> > Path:
> https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051
>
> > 26.5 How To Create 8051 bin file
>
> Please include stuff like this in the help text to make it more obvious.
>

OK. I will add in help text.

> > > > +struct regs_moon0 {
> > > > + u32 stamp; /* 00 */
> > > > + u32 clken[10]; /* 01~10 */
> > > > + u32 gclken[10]; /* 11~20 */
> > > > + u32 reset[10]; /* 21~30 */
> > > > + u32 sfg_cfg_mode; /* 31 */
> > >
> > > What are these comments numbering?
> > >
> >
> > regs_moon0 is Group 0 moon register.
> > The Group0 moon register range is 0x9c00000~0x9c00007F
>
> > /*00*/: 0x9c000000~0x9c000003
> > /*01~10*/:0x9c000004~0x9c00002b
> > /*11~20*/:0x9c00002c~0x9c000053
> > /*21~30*/:0x9c000054~0x9c00007b
> > /*31*/:0x9c00007c~0x9c00007f
>
> Ok, so the number is just the offset into a memory location? Please be
> specific.
>

OK, I will modify it.

>
> >
> > > > +};
> > > > +
> > > > +struct regs_iop {
> > > > + u32 iop_control;/* 00 */
> > > > + u32 iop_reg1;/* 01 */
> > > > + u32 iop_bp;/* 02 */
> > > > + u32 iop_regsel;/* 03 */
> > > > + u32 iop_regout;/* 04 */
> > > > + u32 iop_reg5;/* 05 */
> > > > + u32 iop_resume_pcl;/* 06 */
> > > > + u32 iop_resume_pch;/* 07 */
> > > > + u32 iop_data0;/* 08 */
> > > > + u32 iop_data1;/* 09 */
> > > > + u32 iop_data2;/* 10 */
> > > > + u32 iop_data3;/* 11 */
> > > > + u32 iop_data4;/* 12 */
> > > > + u32 iop_data5;/* 13 */
> > > > + u32 iop_data6;/* 14 */
> > > > + u32 iop_data7;/* 15 */
> > > > + u32 iop_data8;/* 16 */
> > > > + u32 iop_data9;/* 17 */
> > > > + u32 iop_data10;/* 18 */
> > > > + u32 iop_data11;/* 19 */
> > > > + u32 iop_base_adr_l;/* 20 */
> > > > + u32 iop_base_adr_h;/* 21 */
> > > > + u32 memory_bridge_control;/* 22 */
> > > > + u32 iop_regmap_adr_l;/* 23 */
> > > > + u32 iop_regmap_adr_h;/* 24 */
> > > > + u32 iop_direct_adr;/* 25*/
> > > > + u32 reserved[6];/* 26~31 */
> > >
> > > Same here, what are these numbers?
> > >
> > > And why are they not lined up like the previous structure?
> > >
> >
> > Sorry, I don't understand what you mean. Isn't this a struct?
>
>
> Your comments are not lined up the same way the structure above has them.
>

OK, I will modify it.

> > > > + struct mutex write_lock;
> > > > + void *iop_regs;
> > > > + void *pmc_regs;
> > > > + void *moon0_regs;
> > >
> > > Why void pointers? You created structures above, use them!
> > >
> >
> > Because I received "Reported-by: kernel test robot <lkp@xxxxxxxxx>",
> warmming message. As follows:
> > sparse warnings: (new ones prefixed by >>)
>
> > drivers/misc/sunplus_iop.c:94:39: sparse: sparse: cast removes address
> space '__iomem' of expression
> > drivers/misc/sunplus_iop.c:95:43: sparse: sparse: cast removes address
> space '__iomem' of expression
> > >> drivers/misc/sunplus_iop.c:100:16: sparse: sparse: incorrect type in
> argument 1 (different address spaces) @@ expected void *p @@
> got void [noderef] __iomem *[assigned] iop_kernel_base @@
>
> > drivers/misc/sunplus_iop.c:100:16: sparse: expected void *p
>
> > drivers/misc/sunplus_iop.c:100:16: sparse: got void [noderef]
> __iomem *[assigned] iop_kernel_base
>
>
> Ah, so these are actual pointers to the iomem memory? Or pointers to the
> structures you have copied out? It is not obvious.
>
> > > > + reg = readl(&p_moon0_reg->reset[0]);
> > > > + reg |= 0x10;
> > > > + writel(reg, (&p_moon0_reg->reset[0]));
> > > > + reg &= ~(0x10);
> > > > + writel(reg, (&p_moon0_reg->reset[0]));
> > > > +
> > > > + writel(0x00ff0085, (iop->moon0_regs + 32 * 4 * 1 + 4 * 1));
> > > > +
> > > > + reg = readl(iop->moon0_regs + 32 * 4 * 1 + 4 * 2);
> > > > + reg |= 0x08000800;
> > > > + writel(reg, (iop->moon0_regs + 32 * 4 * 1 + 4 * 2));
> > > > +
> > > > + reg = readl(&p_iop_reg->iop_control);
> > > > + reg |= 0x0200;//disable watchdog event reset IOP
> > > > + writel(reg, &p_iop_reg->iop_control);
> > > > +
> > > > + reg = (iop->iop_mem_start & 0xFFFF);
> > > > + writel(reg, &p_iop_reg->iop_base_adr_l);
> > > > + reg = (iop->iop_mem_start >> 16);
> > > > + writel(reg, &p_iop_reg->iop_base_adr_h);
> > > > +
> > > > + reg = readl(&p_iop_reg->iop_control);
> > > > + reg &= ~(0x01);
> > > > + writel(reg, &p_iop_reg->iop_control);
> > > > +
> > > > + writel(WAKEUP_PIN, &p_iop_reg->iop_data0);
> > > > + writel(iop->gpio_wakeup, &p_iop_reg->iop_data1);
> > > > +
> > > > + ret = readl_poll_timeout(&p_iop_reg->iop_data2, value,
> > > > + (value & IOP_READY) == IOP_READY, 1000, 10000);
> > > > + if (ret) {
> > > > + dev_err(dev, "timed out\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + writel(RISC_READY, &p_iop_reg->iop_data2);
> > > > + writel(0x00, &p_iop_reg->iop_data5);
> > > > + writel(0x60, &p_iop_reg->iop_data6);
> > > > +
> > > > + ret = readl_poll_timeout(&p_iop_reg->iop_data7, value,
> > > > + value == 0xaaaa, 1000, 10000);
> > > > + if (ret) {
> > > > + dev_err(dev, "timed out\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + writel(0xdd, &p_iop_reg->iop_data1);//8051 bin file call Ultra
> > > > +low
> > > function.
> > > > + mdelay(10);
> > >
> > > Where did 10 come from? How do you know that is correct?
> > >
> >
> > I need time to move stanby.bin code 16K data from SDRAM to 8051's icache.
>
> > 10msec should be enough
>
> Please document it, as it looks like the data was already sent so there should
> not be any need for a delay on the Linux side.
>
> Or do a read which will ensure that the data is there, right?
>

Let me explain:
1.
writel(0xdd, &p_iop_reg->iop_data1);//8051 bin file call Ultra low function < -----when system linux kernel wrtie mailbox1 iop_data1=0xdd.
8051's standby code see mailbox1=0xdd.
I will move stanby.bin code 16K data from SDRAM to 8051's icache in 8051's standby code.
I will turn off power through the 8051 register(DEF_PWR_EN_0=0) in 8051's standby code.
2.
mdelay(10);<------- When the execution is here, the system linux kernel is about to be powered off
The purpose: Do not let the system linux kernel continue to run other programs

> > > > +static void sp_iop_platform_driver_shutdown(struct
> > > > +platform_device
> > > > +*pdev) {
> > > > + struct sp_iop *iop = platform_get_drvdata(pdev);
> > > > + struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;
> > > > + unsigned int value;
> > > > +
> > > > + sp_iop_standby_mode(iop);
> > > > + mdelay(10);
> > >
> > > Why sleep on shutdown?
> > >
> >
> > I need time to switch from normal.bin code to standby.bin code.
>
>
> Then why does the function call not wait until that happens? Why wait here?
>

sp_iop_standby_mode(iop); <--- I need ensure to switch from normal.bin code to standby.bin code.
mdelay(10); <--- I will remove this delay time. I will read the data to confirm to replace the delay time

Thanks

Next message: Wolfram Sang: "Re: [RFC Patch v2 0/3] I2C statistics as sysfs attributes"
Previous message: Bartosz Golaszewski: "Re: [PATCH] gpio: max3191x: Use bitmap_free() to free bitmap"
In reply to: Greg KH: "Re: [PATCH v6 2/2] misc: Add iop driver for Sunplus SP7021"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]