Message-ID: <CAK8P3a3saiuh8i_PNmfyeCnkTqbONbJ3AeTwa0K6f5Q=saZBBg@mail.gmail.com>
Date:   Wed, 17 Nov 2021 09:27:50 +0100
From:   Arnd Bergmann <arnd@...db.de>
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@...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.

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

> +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.

> 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.

> +//#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.

> +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.

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.

> +#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.

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

> 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.

> 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.


> 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.

> +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.

> +#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.

> +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.

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

       Arnd

Powered by blists - more mailing lists