Message-ID: <2d0a98882f8442df922f129c7894aa48@sphcmbx02.sunplus.com.tw>
Date:   Fri, 19 Nov 2021 02:27:21 +0000
From:   Tony Huang 黃懷厚 <tony.huang@...plus.com>
To:     Arnd Bergmann <arnd@...db.de>,
        Tony Huang <tonyhuang.sunplus@...il.com>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "derek.kiernan@...inx.com" <derek.kiernan@...inx.com>,
        "dragan.cvetic@...inx.com" <dragan.cvetic@...inx.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Wells Lu 呂芳騰 <wells.lu@...plus.com>
Subject: RE: [PATCH 2/2] misc: Add iop driver for Sunplus SP7021

Dear Arnd:

> -----Original Message-----
> From: Arnd Bergmann <arnd@...db.de>
> Sent: Wednesday, November 17, 2021 4:28 PM
> To: Tony Huang <tonyhuang.sunplus@...il.com>
> Cc: robh+dt@...nel.org; devicetree@...r.kernel.org;
> linux-kernel@...r.kernel.org; derek.kiernan@...inx.com;
> dragan.cvetic@...inx.com; arnd@...db.de; gregkh@...uxfoundation.org; Wells
> Lu 呂芳騰 <wells.lu@...plus.com>; Tony Huang 黃懷厚
> <tony.huang@...plus.com>
> Subject: Re: [PATCH 2/2] misc: Add iop driver for Sunplus SP7021
> 
> On Wed, Nov 17, 2021 at 7:48 AM Tony Huang
> <tonyhuang.sunplus@...il.com> wrote:
> >
> > Add iop driver for Sunplus SP7021
> >
> > Signed-off-by: Tony Huang <tony.huang@...plus.com>
> 
> A driver like this needs a long description of multiple paragraphs to explain
> what it does, why you need a custom user space interface etc.

I will add long description.

> 
> > +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.
> > diff --git a/drivers/misc/iop/Makefile b/drivers/misc/iop/Makefile new
> > file mode 100644 index 0000000..cb67634
> > --- /dev/null
> > +++ b/drivers/misc/iop/Makefile
> > @@ -0,0 +1,13 @@
> > +#
> > +# Makefile for the IOP module drivers.
> > +#
> > +
> > +       # call from kernel build system
> > +
> > +       obj-$(CONFIG_SUNPLUS_IOP) += sunplus_iop.o
> > +       obj-$(CONFIG_SUNPLUS_IOP) += hal_iop.o
> > +       obj-$(CONFIG_SUNPLUS_IOP) += iopnormal.o
> > +       obj-$(CONFIG_SUNPLUS_IOP) += iopstandby.o
> 
> Remove the extra whitespace here

I will remove it.

> 
> > +clean:
> > +       rm -rf *.o *~ core .depend .*.cmd *.ko *.mod.c .tmp_versions
> > \ No newline at end of file
> 
> Replace the explicit 'rm' command with Kbuild listings for these files.
> Most of the files are already covered by the regular 'clean' target, so you
> should only need special handling for stuff you build separately as well.

I will modify it.

> 
> > diff --git a/drivers/misc/iop/hal_iop.c b/drivers/misc/iop/hal_iop.c
> > new file mode 100644 index 0000000..6a72c8b
> > --- /dev/null
> > +++ b/drivers/misc/iop/hal_iop.c
> > @@ -0,0 +1,495 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include "hal_iop.h"
> 
> One fundamental rule for Linux kernel drivers is that we assume that we know
> which OS we are running on, so there is no need for abstracting the operating
> system away from the driver. Please fold your 'hal' into the top-level driver
> accordingly and remove any unneded indirections between the two. I suppose
> you could have everything in one file if after you remove the firmware from
> the driver, and no longer require any headers.

I will modify it.

> 
> > +//#define DEBUG_MESSAGE
> > +//#define early_printk
> > +
> > +#define IOP_KDBG_INFO
> > +#define IOP_FUNC_DEBUG
> > +#define IOP_KDBG_ERR
> > +#ifdef IOP_KDBG_INFO
> > +       #define FUNC_DEBUG()    pr_info("K_IOP: %s(%d)\n", __func__,
> __LINE__)
> > +#else
> > +       #define FUNC_DEBUG()
> > +#endif
> > +
> > +#ifdef IOP_FUNC_DEBUG
> > +#define DBG_INFO(fmt, args ...)        pr_info("K_IOP: " fmt, ## args)
> > +#else
> > +#define DBG_INFO(fmt, args ...)
> > +#endif
> > +
> > +#ifdef IOP_KDBG_ERR
> > +#define DBG_ERR(fmt, args ...) pr_err("K_IOP: " fmt, ## args) #else
> > +#define DBG_ERR(fmt, args ...) #endif
> 
> These should all go away, try using dev_dbg() / dev_info() / dev_err()
> preferably.

I will modify it.

> 
> > +void hal_iop_init(void __iomem *iopbase) {
> > +       struct regs_iop_t *pIopReg = (struct regs_iop_t *)iopbase;
> > +       unsigned long *IOP_base_for_normal = (unsigned long
> *)SP_IOP_RESERVE_BASE;
> > +       unsigned char *IOP_kernel_base;
> > +       unsigned int reg;
> > +
> > +       IOP_kernel_base = (unsigned char *)ioremap((unsigned
> long)IOP_base_for_normal,
> > +               NORMAL_CODE_MAX_SIZE);
> > +
> > +       memset((unsigned char *)IOP_kernel_base, 0,
> NORMAL_CODE_MAX_SIZE);
> > +       memcpy((unsigned char *)IOP_kernel_base, IopNormalCode,
> NORMAL_CODE_MAX_SIZE);
> > +       writel(0x00100010, (void __iomem *)(B_SYSTEM_BASE + 32*4*0 +
> > + 4*1));
> 
> All the type casts should be removed after you use the correct types:
> 'void __iomem *'
> for MMIO tokens, and 'void *' for addressable memory. You can use the
> 'sparse' tool by running 'make C=1' to check for any mismatched __iomem
> annotations.

I will modify it.

> 
> To write to an __iomem area, you can use memcpy_to_io() and memset_io()
> 
> > diff --git a/drivers/misc/iop/hal_iop.h b/drivers/misc/iop/hal_iop.h
> > new file mode 100644 index 0000000..d83f9ea
> > --- /dev/null
> > +++ b/drivers/misc/iop/hal_iop.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later*/
> > +
> > +#ifndef __IOP_HAL_H__
> > +#define __IOP_HAL_H__
> > +
> > +#include <linux/types.h>
> > +#include <linux/module.h>
> > +#include "reg_iop.h"
> > +
> > +void hal_iop_init(void __iomem *iopbase); void
> > +hal_iop_load_normal_code(void __iomem *iopbase); void
> > +hal_iop_load_standby_code(void __iomem *iopbase); void
> > +hal_iop_normalmode(void __iomem *iopbase); void
> > +hal_iop_standbymode(void __iomem *iopbase); void
> > +hal_iop_get_iop_data(void __iomem *iopbase); void
> > +hal_iop_set_iop_data(void __iomem *iopbase, unsigned int num,
> > +unsigned int value); void hal_gpio_init(void __iomem *iopbase,
> > +unsigned char gpio_number); void hal_iop_suspend(void __iomem
> > +*iopbase, void __iomem *ioppmcbase); void hal_iop_shutdown(void
> > +__iomem *iopbase, void __iomem *ioppmcbase); void
> hal_iop_S1mode(void
> > +__iomem *iopbase); void hal_iop_set_reserve_base(void __iomem
> > +*iopbase); void hal_iop_set_reserve_size(void __iomem *iopbase);
> 
> When you fold those into the actual driver but decide to keep the functions
> separate, try to always pass a pointer to the device instance as the first
> argument. This is the way we do object oriented programming in the kernel.

I will modify it.

> 
> > +#define NORMAL_CODE_MAX_SIZE 0X10000
> > +#define STANDBY_CODE_MAX_SIZE 0x4000
> > +extern const unsigned char IopNormalCode[]; extern const unsigned
> > +char IopStandbyCode[]; extern bool iop_code_mode; extern unsigned
> > +long SP_IOP_RESERVE_BASE; extern unsigned long SP_IOP_RESERVE_SIZE;
> > +extern unsigned int RECEIVE_CODE_SIZE; extern unsigned char
> > +NormalCode[]; extern unsigned char StandbyCode[]; #endif /*
> > +__IOP_HAL_H__ */
> 
> There should generally be no global variables. Instead, these should go into
> per-instance structures.

I will modify it.

> 
> Regarding the naming, avoid CamelCase and use e.g. 'iop_standby_code'
> instead of 'IopStandbyCode'.

I will modify it.

> 
> > diff --git a/drivers/misc/iop/iop_ioctl.h
> > b/drivers/misc/iop/iop_ioctl.h new file mode 100644 index
> > 0000000..195388b
> > --- /dev/null
> > +++ b/drivers/misc/iop/iop_ioctl.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later*/
> > +/*
> > + *
> > + *iop_ioctl.h
> > + *
> > + */
> > +
> > +#ifndef _IOP_IOCTL_H
> > +#define _IOP_IOCTL_H
> > +
> > +#include <linux/ioctl.h>
> > +
> > +struct ioctl_cmd {
> > +               unsigned int reg;
> > +               unsigned int offset;
> > +               unsigned int val;
> > +};
> > +
> > +#define IOC_MAGIC 'i'
> > +
> > +#define IOCTL_VALSET _IOW(IOC_MAGIC, 1, struct ioctl_cmd) #define
> > +IOCTL_VALGET _IOR(IOC_MAGIC, 2, struct ioctl_cmd)
> > +
> > +#endif
> 
> ioctl interfaces are usually the hardest part of a driver, the best way to do this
> is to completely avoid inventing your own interfaces and using what the kernel
> already has, extending the existing interfaces if possible.
> 
> If you end up having to add your own ioctls, do high-level functions rather than
> low-level register access, and create a separate ioctl command for each
> action.

I will modify it.

> 
> > diff --git a/drivers/misc/iop/iopnormal.c
> > b/drivers/misc/iop/iopnormal.c new file mode 100644 index
> > 0000000..a49a376
> > --- /dev/null
> > +++ b/drivers/misc/iop/iopnormal.c
> > @@ -0,0 +1,4106 @@
> > +const unsigned char IopNormalCode[] = { 0x02, 0x1E, 0x00, 0x02, 0xFF,
> > +0xFF, 0x2E, 0x05, 0x02, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xB0, 0x05,
> > +0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x02, 0xFF, 0xFF,
> > +0xFF, 0x08, 0x75, 0xCD, 0x05, 0x00, 0x09, 0x75, 0x04, 0x02, 0x8F,
> > +0x81, 0x75, 0x52, 0x78, 0x73, 0x04, 0x13, 0xEF, 0xFF, 0xE6,
> 
> As I understand, this is the binary firmware that gets loaded into the iop.
> 
> The way we normally handle firmware loading in the kernel is to use the
> 'request_firmware()' interface from the driver to load the binary from a file in
> the root file system.

I will try to use request_firmware().

> 
> 
> > diff --git a/drivers/misc/iop/reg_iop.h b/drivers/misc/iop/reg_iop.h
> > new file mode 100644 index 0000000..829aa76
> > --- /dev/null
> > +++ b/drivers/misc/iop/reg_iop.h
> > @@ -0,0 +1,93 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later*/ #ifndef __REG_IOP_H__
> > +#define __REG_IOP_H__ #ifdef CONFIG_SOC_SP7021 #include
> > +<mach/io_map.h> #endif
> 
> There should not be any conditional on the type of the SOC here, the driver
> object needs to work in a kernel that includes support for multiple SoCs.
> 
> Note that there are no mach/*.h headers for modern SoCs any more, so the
> platform code needs to be updated accordingly. In particular, any MMIO
> register locations need to come from the devicetree, not from a hardcoded
> header.

I will modify it.

> 
> > +struct regs_iop_moon0_t {
> > +       unsigned int stamp;/* 00 */
> > +       unsigned int clken[10];/* 01~10 */
> > +       unsigned int gclken[10];/* 11~20 */
> > +       unsigned int reset[10];/* 21~30 */
> > +       unsigned int sfg_cfg_mode;/* 31 */ };
> 
> Drop the '_t' suffix on the structure names, those are commonly associated
> with names for typedefs, not structures.
> 
> Most driver writers find it easier to define register indexes as macros rather
> than structure definitions, but if all registers are 32-bit wide, then this works as
> well.
> 
> Using 'u32' as the type instead of 'unsigned int' would make this more explicit.

I will modify it.

> 
> > +#ifdef IOP_KDBG_INFO
> > +#define FUNC_DEBUG()   pr_info("K_IOP: %s(%d)\n", __func__, __LINE__)
> > +#else
> > +#define FUNC_DEBUG()
> > +#endif
> > +
> > +#ifdef IOP_FUNC_DEBUG
> > +#define DBG_INFO(fmt, args ...)        pr_info("K_IOP: " fmt, ## args)
> > +#else
> > +#define DBG_INFO(fmt, args ...)
> > +#endif
> > +
> > +#ifdef IOP_KDBG_ERR
> > +#define DBG_ERR(fmt, args ...) pr_err("K_IOP: " fmt, ## args) #else
> > +#define DBG_ERR(fmt, args ...) #endif
> > +/* ----------------------------------------------------------------------------------------------
> */
> > +#define IOP_REG_NAME           "iop"
> > +#define IOP_PMC_REG_NAME          "iop_pmc"
> > +
> > +#define DEVICE_NAME                    "sunplus,sp7021-iop"
> 
> Remove all those and open-code the correct contents in the users.

I will remove it.

> 
> > +static ssize_t setgpio_store(struct device *dev, struct device_attribute *attr,
> > +       const char *buf, size_t count) {
> > +       int ret = count;
> > +       unsigned char num[1];
> > +       unsigned int setnum;
> > +       unsigned long val;
> > +       ssize_t status;
> > +
> > +       DBG_INFO("iop_store_setgpio\n");
> > +       num[0] = buf[0];
> > +       status = kstrtoul(buf, 16, &val);
> > +       if (status)
> > +               return status;
> > +       setnum = val;
> > +       DBG_INFO("set gpio number = %x\n", IOP_GPIO);
> > +       hal_gpio_init(iop->iop_regs, IOP_GPIO);
> > +       return ret;
> > +}
> 
> GPIO access should be handled through the gpio subsystem by creating an
> instance of a gpio_chip, which provides interfaces for both in-kernel and
> user-space users.

I will modify it.

> 
> > +static ssize_t S1mode_show(struct device *dev, struct device_attribute
> *attr,
> > +       char *buf)
> > +{
> > +       ssize_t len = 0;
> > +
> > +       hal_iop_standbymode(iop->iop_regs);
> > +       mdelay(10);//Need time to update iop_data
> > +       hal_iop_S1mode(iop->iop_regs);
> > +       return len;
> > +}
> > +
> > +static ssize_t S1mode_store(struct device *dev, struct device_attribute
> *attr,
> > +       const char *buf, size_t count) {
> > +       ssize_t len = 0;
> > +
> > +       return len;
> > +}
> 
> Each new device attribute needs to be documented in Documentation/ABI/

I will documented for new attribute in Documentation/ABI/.

> 
>        Arnd

Powered by blists - more mailing lists