lkml.org 
[lkml]   [2022]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021
From
On 12/03/2022 17:16, Tony Huang wrote:
> This driver is load 8051 bin code.
> The IOP core is DQ8051, so also named IOP8051.
> Need Install DQ8051, The DQ8051 bin file generated by keil C.
> We will provide users with 8051 normal and power off 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
> Users can follow the operation steps to generate normal.bin and
> poweroff.bin.
> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338
> /26.+IOP8051 26.5?How To Create 8051 bin file
>
> PMC in power management purpose:
> Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> When the power off command is executed.
> The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
> power-off of the system.
>
> Signed-off-by: Tony Huang <tonyhuang.sunplus@gmail.com>
> ---
> Changes in v11:
> - Addressed comments from Arnd Bergmann.

How did you address Arnd's comments about splitting the driver to proper
parts? drivers/clk and drivers/power/reset?

> - Addressed comments from krzysztof.
>
> MAINTAINERS | 1 +
> drivers/misc/Kconfig | 23 +++
> drivers/misc/Makefile | 1 +
> drivers/misc/sunplus_iop.c | 411 +++++++++++++++++++++++++++++++++++++++++++++

The driver looks like SoC specific driver. Why did you put it in
drivers/misc/, not in usual place - drivers/soc/? sp_iop_poweroff is
still here.

> 4 files changed, 436 insertions(+)
> create mode 100644 drivers/misc/sunplus_iop.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d64c8ed..c282e95 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18246,6 +18246,7 @@ SUNPLUS IOP DRIVER
> M: Tony Huang <tonyhuang.sunplus@gmail.com>
> S: Maintained
> F: Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> +F: drivers/misc/sunplus_iop.c
>
> SUPERH
> M: Yoshinori Sato <ysato@users.sourceforge.jp>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 0f5a49f..e5f32d8 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -470,6 +470,29 @@ config HISI_HIKEY_USB
> switching between the dual-role USB-C port and the USB-A host ports
> using only one USB controller.
>
> +config SUNPLUS_IOP
> + tristate "Sunplus IOP support"
> + default ARCH_SUNPLUS
> + help
> + This driver is load 8051 bin code.
> + The IOP core is DQ8051, so also named IOP8051.
> + Need Install DQ8051, The DQ8051 bin file generated by keil C.
> + We will provide users with 8051 normal and power off 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
> + Users can follow the operation steps to generate normal.bin and poweroff.bin.
> + Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338/26.+IOP8051
> + 26.5?How To Create 8051 bin file
> +
> + PMC in power management purpose:
> + Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
> + When the power off command is executed.
> + The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
> + power-off of the system.
> +
> + This driver can also be built as a module. If so, the module
> + will be called sunplus_iop.
> +
> source "drivers/misc/c2port/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index a086197..eafeab6 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_DW_XDATA_PCIE) += dw-xdata-pcie.o
> obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o
> obj-$(CONFIG_OCXL) += ocxl/
> obj-$(CONFIG_BCM_VK) += bcm-vk/
> +obj-$(CONFIG_SUNPLUS_IOP) += sunplus_iop.o
> obj-y += cardreader/
> obj-$(CONFIG_PVPANIC) += pvpanic/
> obj-$(CONFIG_HABANA_AI) += habanalabs/
> diff --git a/drivers/misc/sunplus_iop.c b/drivers/misc/sunplus_iop.c
> new file mode 100644
> index 0000000..5bdce5e
> --- /dev/null
> +++ b/drivers/misc/sunplus_iop.c
> @@ -0,0 +1,411 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * The IOP driver for Sunplus SP7021
> + *
> + * Copyright (C) 2021 Sunplus Technology Inc.
> + *
> + * All Rights Reserved.
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/firmware.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +
> +/* IOP register offset */
> +#define IOP_CONTROL 0x00
> +#define IOP_DATA0 0x20
> +#define IOP_DATA1 0x24
> +#define IOP_DATA2 0x28
> +#define IOP_DATA3 0x2c
> +#define IOP_DATA4 0x30
> +#define IOP_DATA5 0x34
> +#define IOP_DATA6 0x38
> +#define IOP_DATA7 0x3c
> +#define IOP_DATA8 0x40
> +#define IOP_DATA9 0x44
> +#define IOP_DATA10 0x48
> +#define IOP_DATA11 0x4c
> +#define IOP_BASE_ADR_L 0x50
> +#define IOP_BASE_ADR_H 0x54
> +
> +/* PMC register offset */
> +#define IOP_PMC_TIMER 0x00
> +#define IOP_PMC_CTRL 0x04
> +#define IOP_XTAL27M_PASSWORD_I 0x08
> +#define IOP_XTAL27M_PASSWORD_II 0x0c
> +#define IOP_XTAL32K_PASSWORD_I 0x10
> +#define IOP_XTAL32K_PASSWORD_II 0x14
> +#define IOP_CLK27M_PASSWORD_I 0x18
> +#define IOP_CLK27M_PASSWORD_II 0x1c
> +#define IOP_PMC_TIMER2 0x20
> +
> +/* Max size of poweroff.bin that can be received */
> +#define POWEROFF_CODE_MAX_SIZE 0x4000
> +
> +/* 8051 informs linux kerenl. 8051 has been switched to poweroff.bin code. */
> +#define IOP_READY 0x0004
> +#define RISC_READY 0x0008
> +
> +/* System linux kernel tells 8051 which gpio pin to wake-up through. */
> +#define WAKEUP_PIN 0xFE02
> +
> +/*
> + * There are 3 power domains in SP7021, AO domain, IOP domain,
> + * Default domain. Default domain is linux kernel system.
> + * System linux kernel tells 8051 to execute power off.
> + */
> +#define DEFAULT_DOMAIN_POWEROFF 0x5331 /* AO&IOP domain on, Default domain off */
> +#define DEFAULT_AND_IOP_DOMAIN_POWEROFF 0x5333 /* AO domain on, IOP&Default domain off */
> +
> +struct sp_iop {
> + struct device *dev;
> + struct clk *iopclk;
> + struct reset_control *rstc;
> + void __iomem *iop_regs;
> + void __iomem *pmc_regs;
> + void __iomem *moon0_regs;
> + int irq;
> + int gpio_wakeup;
> + resource_size_t iop_mem_start;
> + resource_size_t iop_mem_size;
> + unsigned char bin_code_mode;
> +};
> +
> +static struct sp_iop *iop_poweroff;
> +
> +static void sp_iop_load_normal_code(struct sp_iop *iop)
> +{
> + const struct firmware *fw;
> + void __iomem *iop_kernel_base;
> + unsigned int reg, err;
> +
> + err = request_firmware(&fw, "normal.bin", iop->dev);
> + if (err)
> + dev_err(iop->dev, "get bin file error\n");
> +
> + iop_kernel_base = ioremap(iop->iop_mem_start, fw->size);
> + memset(iop_kernel_base, 0, fw->size);
> + memcpy(iop_kernel_base, fw->data, fw->size);
> + release_firmware(fw);
> +
> + clk_prepare_enable(iop->iopclk);

where do you disable the clock?

> +
> + reg = readl(iop->iop_regs + IOP_CONTROL);
> + reg |= 0x01;
> + writel(reg, iop->iop_regs + IOP_CONTROL);
> +

(...)

> +static int sp_iop_get_resources(struct platform_device *pdev, struct sp_iop *p_sp_iop_info)
> +{
> + struct resource *r;
> +
> + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop");
> + p_sp_iop_info->iop_regs = devm_ioremap_resource(&pdev->dev, r);
> + if (IS_ERR(p_sp_iop_info->iop_regs)) {
> + dev_err(&pdev->dev, "ioremap fail\n");
> + return PTR_ERR(p_sp_iop_info->iop_regs);
> + }
> +
> + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iop_pmc");
> + p_sp_iop_info->pmc_regs = devm_ioremap_resource(&pdev->dev, r);
> + if (IS_ERR(p_sp_iop_info->pmc_regs)) {
> + dev_err(&pdev->dev, "ioremap fail\n");
> + return PTR_ERR(p_sp_iop_info->pmc_regs);
> + }
> +
> + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon0");
> + p_sp_iop_info->moon0_regs = devm_ioremap_resource(&pdev->dev, r);
> + if (IS_ERR(p_sp_iop_info->moon0_regs)) {
> + dev_err(&pdev->dev, "ioremap fail\n");
> + return PTR_ERR(p_sp_iop_info->moon0_regs);
> + }
> + return 0;

https://lore.kernel.org/all/16d98dfa-66f9-f9a4-08c9-d68d78c33b23@canonical.com/
You received a comment about adding blank lines before return. This has
to be done everywhere.

> +}
> +
> +static int sp_iop_platform_driver_probe(struct platform_device *pdev)

Do not add "platform_driver" suffix to functions. "_probe" is enough.

> +{
> + int ret = -ENXIO;
> + int rc;
> + struct sp_iop *iop;
> + struct device_node *memnp;
> + struct resource mem_res;
> +
> + iop = devm_kzalloc(&pdev->dev, sizeof(struct sp_iop), GFP_KERNEL);
> + if (!iop) {
> + ret = -ENOMEM;
> + goto fail_kmalloc;
> + }
> +
> + ret = sp_iop_get_resources(pdev, iop);
> +
> + /* Get reserve address. */
> + memnp = of_parse_phandle(pdev->dev.of_node, "memory-region", 0);
> + if (!memnp) {
> + dev_err(&pdev->dev, "no memory-region node\n");
> + return -EINVAL;
> + }
> +
> + rc = of_address_to_resource(memnp, 0, &mem_res);
> + of_node_put(memnp);
> + if (rc) {
> + dev_err(&pdev->dev, "failed to translate memory-region to a resource\n");
> + return -EINVAL;
> + }
> +
> + iop->dev = &pdev->dev;
> + iop->iop_mem_start = mem_res.start;
> + iop->iop_mem_size = resource_size(&mem_res);
> + iop->iopclk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(iop->iopclk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(iop->iopclk),
> + "devm_clk_get fail\n");

Clocks are not documented but required in bindings?

> +
> + /*
> + * 8051 has two bin files, normal.bin and poweroff.bin in rootfs.
> + * Normally, 8051 executes normal.bin code. Normal.bin code size can exceed 16K.
> + */
> + sp_iop_load_normal_code(iop);
> +
> + platform_set_drvdata(pdev, iop);
> + iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node, "iop-wakeup", 0);

It's not in the bindings. Do not add undocumented properties. Everything
has to be in the bindings. I don't see usage of generic wakeup-gpios, so
this all looks untested. You didn't run this code, did you?

> + iop_poweroff = iop;
> + pm_power_off = sp_iop_poweroff;
> +
> + return 0;
> +
> +fail_kmalloc:
> + return ret;
> +}
> +
> +static int sp_iop_remove(struct platform_device *pdev)
> +{
> + pm_power_off = NULL;
> + return 0;
> +}
> +
> +static const struct of_device_id sp_iop_of_match[] = {
> + { .compatible = "sunplus,sp7021-iop" },
> + { /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, sp_iop_of_match);
> +
> +static struct platform_driver sp_iop_platform_driver = {
> + .probe = sp_iop_platform_driver_probe,
> + .remove = sp_iop_remove,
> + .driver = {
> + .name = "sunplus,sp7021-iop",
> + .of_match_table = sp_iop_of_match,
> + }
> +};
> +
> +module_platform_driver(sp_iop_platform_driver);
> +
> +MODULE_AUTHOR("Tony Huang <tonyhuang.sunplus@gmail.com>");
> +MODULE_DESCRIPTION("Sunplus IOP Driver");
> +MODULE_LICENSE("GPL v2");


Best regards,
Krzysztof

\
 
 \ /
  Last update: 2022-03-13 10:44    [W:1.505 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site