| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <42f5f710077b40d7b6fde45789f46732@sphcmbx02.sunplus.com.tw>
Date: Mon, 14 Mar 2022 06:08:24 +0000
From: Tony Huang 黃懷厚 <tony.huang@...plus.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
Tony Huang <tonyhuang.sunplus@...il.com>,
"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>,
"arnd@...db.de" <arnd@...db.de>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC: Wells Lu 呂芳騰 <wells.lu@...plus.com>
Subject: RE: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021
Dear Krzysztof:
> 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@...il.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?
>
drivers/clk : SP7021 system has clock device driver (clk-sp7021.c).
So I set the IOP clock through the following function.
clk_prepare_enable(iop->iopclk);
clk_disable_unprepare(iop->iopclk);
drivers/power/reset : SP7021 system does not have a power off device driver.
> > - 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/?
8051 is designed for processing I/O events, like receiving IR signal from remote controller,
taking care of power on or off requests from RTC, or other hardware events of external peripherals
even when power of main system is off.
So I put it in drivers/misc.
> sp_iop_poweroff is still here.
sp_iop_poweroff(): SP7021 system does not have a power off device driver.
I have to put it 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@...il.com>
> > S: Maintained
> > F: Documentation/devicetree/bindings/misc/sunplus,iop.yaml
> > +F: drivers/misc/sunplus_iop.c
> >
> > SUPERH
> > M: Yoshinori Sato <ysato@...rs.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?
I will add disable clock in sp_iop_remove().
Below is my modification
static int sp_iop_remove(struct platform_device *pdev)
{
struct sp_iop *iop = iop_poweroff;
pm_power_off = NULL;
clk_disable_unprepare(iop->iopclk);
return 0;
}
> > +
> > + 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.
OK, I will modify it. Added blank line before return everywhere.
> > +}
> > +
> > +static int sp_iop_platform_driver_probe(struct platform_device *pdev)
>
> Do not add "platform_driver" suffix to functions. "_probe" is enough.
>
Below is my modification:
static int sp_iop_probe(struct platform_device *pdev)
> > +{
> > + 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?
>
I will added clocks in properties.
> > +
> > + /*
> > + * 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?
>
I wrote the wrong name.
Below is my modification:
iop->gpio_wakeup = of_get_named_gpio(pdev->dev.of_node, "wakeup-gpios", 0);
Powered by blists - more mailing lists