On 4/4/19 9:20 PM, Leo Li wrote: > > >> -----Original Message----- >> From: Ioana Ciornei >> Sent: Thursday, April 4, 2019 5:48 AM >> To: Leo Li <leoyang...@nxp.com> >> Cc: linux-kernel@vger.kernel.org; Roy Pledge <roy.ple...@nxp.com> >> Subject: Re: [PATCH] soc: fsl: add DPAA2 console support >> >> On 4/3/19 10:55 PM, Li Yang wrote: >>> On Tue, Mar 26, 2019 at 2:17 PM Ioana Ciornei <ioana.cior...@nxp.com> >> wrote: >>>> >>>> This patch adds DPAA2 MC and AIOP console log support. >>>> >>>> The platform driver probes on the "fsl,dpaa2-console" device tree >>>> node which describes the base firmware address needed in order to >>>> infer the start address of both firmware logs: MC and AIOP. >>>> It then exports two misc char devices which can be used to dump the >>>> needed logs. >>>> >>>> Signed-off-by: Ioana Ciornei <ioana.cior...@nxp.com> >>>> Signed-off-by: Roy Pledge <roy.ple...@nxp.com> >>>> --- >>>> drivers/soc/fsl/Kconfig | 10 ++ >>>> drivers/soc/fsl/Makefile | 1 + >>>> drivers/soc/fsl/dpaa2-console.c | 306 >> ++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 317 insertions(+) >>>> create mode 100644 drivers/soc/fsl/dpaa2-console.c >>>> >>>> diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig index >>>> 61f8e1433d0a..9fe8222c3968 100644 >>>> --- a/drivers/soc/fsl/Kconfig >>>> +++ b/drivers/soc/fsl/Kconfig >>>> @@ -29,4 +29,14 @@ config FSL_MC_DPIO >>>> other DPAA2 objects. This driver does not expose the DPIO >>>> objects individually, but groups them under a service layer >>>> API. >>>> + >>>> +config DPAA2_CONSOLE >>>> + tristate "QorIQ DPAA2 console driver" >>>> + depends on OF && (ARCH_LAYERSCAPE || (COMPILE_TEST && (ARM >> || >>>> +ARM64 || X86_LOCAL_APIC || PPC))) >>> >>> I don't know why FSL_MC driver added there architectures dependency >>> for COMPILE_TEST, but is this driver also only buildable on these >>> architectures? Can we really just remove these dependencies? >>> >> >> The initial reasoning behind the dependency on the COMPILE_TEST config >> was to catch early build problems on any build configuration. >> And yes, the dpaa2-console can be compiled without a problem on these >> architectures. >> >> In this case, I would suggest to leave the dependencies as they are but if >> you >> want I can also remove them. > > Can we keep the COMPILE_TEST and remove the (ARM || ARM64 || X86_LOCAL_APIC > || PPC) if we are not using any architecture specific stuff?
Yes, that should be a better solution. Will send a v2 with all the changes. Thanks, Ioana C > >> >>>> + default y >>>> + help >>>> + Console driver for DPAA2 platforms. Exports 2 char devices, >>>> + /dev/dpaa2_mc_console and /dev/dpaa2_aiop_console, >>>> + which can be used to dump the Management Complex and AIOP >>>> + firmware logs. >>>> endmenu >>>> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile >>>> index 803ef1bfb5ff..57762c9fc7da 100644 >>>> --- a/drivers/soc/fsl/Makefile >>>> +++ b/drivers/soc/fsl/Makefile >>>> @@ -7,3 +7,4 @@ obj-$(CONFIG_QUICC_ENGINE) += qe/ >>>> obj-$(CONFIG_CPM) += qe/ >>>> obj-$(CONFIG_FSL_GUTS) += guts.o >>>> obj-$(CONFIG_FSL_MC_DPIO) += dpio/ >>>> +obj-$(CONFIG_DPAA2_CONSOLE) += dpaa2-console.o >>>> diff --git a/drivers/soc/fsl/dpaa2-console.c >>>> b/drivers/soc/fsl/dpaa2-console.c new file mode 100644 index >>>> 000000000000..21a5e121f87b >>>> --- /dev/null >>>> +++ b/drivers/soc/fsl/dpaa2-console.c >>>> @@ -0,0 +1,306 @@ >>>> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) >>> >>> Probably good to have a one-line description of the driver? >> >> Will add one in the next version. >> >>> >>>> +/* Copyright 2015-2016 Freescale Semiconductor Inc. >>>> + * Copyright 2018 NXP >>>> + */ >>>> + >>>> +#define pr_fmt(fmt) "dpaa2-console: " fmt >>>> + >>>> +#include <linux/module.h> >>>> +#include <linux/of_device.h> >>>> +#include <linux/of_address.h> >>>> +#include <linux/miscdevice.h> >>>> +#include <linux/uaccess.h> >>>> +#include <linux/slab.h> >>>> +#include <linux/fs.h> >>>> +#include <linux/io.h> >>>> + >>>> +MODULE_LICENSE("Dual BSD/GPL"); >>>> +MODULE_AUTHOR("Roy Pledge <roy.ple...@nxp.com>"); >>>> +MODULE_DESCRIPTION("DPAA2 console driver"); >>> >>> These normally go to the bottom of a driver. >> >> Will move them. >> >>> >>>> + >>>> +/* MC firmware base low/high registers indexes */ #define >>>> +MCFBALR_OFFSET 0 #define MCFBAHR_OFFSET 1 >>>> + >>>> +/* Bit masks used to get the most/least significant part of the MC >>>> +base addr */ #define MC_FW_ADDR_MASK_HIGH 0x1FFFF #define >>>> +MC_FW_ADDR_MASK_LOW 0xE0000000 >>>> + >>>> +#define MC_BUFFER_OFFSET 0x01000000 >>>> +#define MC_BUFFER_SIZE (1024 * 1024 * 16) >>>> +#define MC_OFFSET_DELTA MC_BUFFER_OFFSET >>>> + >>>> +#define AIOP_BUFFER_OFFSET 0x06000000 >>>> +#define AIOP_BUFFER_SIZE (1024 * 1024 * 16) >>>> +#define AIOP_OFFSET_DELTA 0 >>>> + >>>> +#define LOG_HEADER_FLAG_BUFFER_WRAPAROUND 0x80000000 >> #define >>>> +LAST_BYTE(a) ((a) & ~(LOG_HEADER_FLAG_BUFFER_WRAPAROUND)) >>>> + >>>> +/* MC and AIOP Magic words */ >>>> +#define MAGIC_MC 0x4d430100 >>>> +#define MAGIC_AIOP 0x41494F50 >>>> + >>>> +struct log_header { >>>> + __le32 magic_word; >>>> + char reserved[4]; >>>> + __le32 buf_start; >>>> + __le32 buf_length; >>>> + __le32 last_byte; >>>> +}; >>>> + >>>> +struct console_data { >>>> + char *map_addr; >>>> + struct log_header *hdr; >>>> + char *start_addr; >>>> + char *end_addr; >>>> + char *end_of_data; >>>> + char *cur_ptr; >>>> +}; >>>> + >>>> +struct resource mc_base_addr; >>>> + >>>> +static inline void adjust_end(struct console_data *cd) { >>>> + u32 last_byte = readl(&cd->hdr->last_byte); >>>> + >>>> + cd->end_of_data = cd->start_addr + LAST_BYTE(last_byte); } >>>> + >>>> +static u64 get_mc_fw_base_address(void) { >>>> + u64 mcfwbase = 0ULL; >>>> + u32 *mcfbaregs; >>>> + >>>> + mcfbaregs = (u32 *)ioremap(mc_base_addr.start, >>>> + resource_size(&mc_base_addr)); >>>> + if (!mcfbaregs) { >>>> + pr_err("could not map MC Firmaware Base registers\n"); >>>> + return -EIO; >>>> + } >>>> + >>>> + mcfwbase = readl(mcfbaregs + MCFBAHR_OFFSET) & >> MC_FW_ADDR_MASK_HIGH; >>>> + mcfwbase <<= 32; >>>> + mcfwbase |= readl(mcfbaregs + MCFBALR_OFFSET) & >> MC_FW_ADDR_MASK_LOW; >>>> + iounmap(mcfbaregs); >>>> + >>>> + pr_debug("MC base address at 0x%016llx\n", mcfwbase); >>>> + return mcfwbase; >>>> +} >>>> + >>>> +static ssize_t dpaa2_console_size(struct console_data *cd) { >>>> + ssize_t size; >>>> + >>>> + if (cd->cur_ptr <= cd->end_of_data) >>>> + size = cd->end_of_data - cd->cur_ptr; >>>> + else >>>> + size = (cd->end_addr - cd->cur_ptr) + >>>> + (cd->end_of_data - cd->start_addr); >>>> + >>>> + return size; >>>> +} >>>> + >>>> +static int dpaa2_generic_console_open(struct inode *node, struct file *fp, >>>> + u64 offset, u64 size, >>>> + u32 expected_magic, >>>> + u32 offset_delta) { >>>> + u32 read_magic, wrapped, last_byte, buf_start, buf_length; >>>> + struct console_data *cd; >>>> + u64 base_addr; >>>> + int err; >>>> + >>>> + cd = kmalloc(sizeof(*cd), GFP_KERNEL); >>>> + if (!cd) >>>> + return -ENOMEM; >>>> + >>>> + base_addr = get_mc_fw_base_address(); >>>> + if (base_addr < 0) >>> >>> We are leaking cd structure here. >> >> Ugh, that's right. Will fix. >> >>> >>>> + return -EIO; >>>> + >>>> + cd->map_addr = ioremap(base_addr + offset, size); >>>> + if (!cd->map_addr) { >>>> + pr_err("cannot map console log memory\n"); >>>> + err = -EIO; >>>> + goto err_ioremap; >>>> + } >>>> + >>>> + cd->hdr = (struct log_header *)cd->map_addr; >>>> + read_magic = readl(&cd->hdr->magic_word); >>>> + last_byte = readl(&cd->hdr->last_byte); >>>> + buf_start = readl(&cd->hdr->buf_start); >>>> + buf_length = readl(&cd->hdr->buf_length); >>>> + >>>> + if (read_magic != expected_magic) { >>>> + pr_warn("expected = %08x, read = %08x\n", >>>> + expected_magic, read_magic); >>>> + err = -EIO; >>>> + goto err_magic; >>>> + } >>>> + >>>> + cd->start_addr = cd->map_addr + buf_start - offset_delta; >>>> + cd->end_addr = cd->start_addr + buf_length; >>>> + >>>> + wrapped = last_byte & LOG_HEADER_FLAG_BUFFER_WRAPAROUND; >>>> + >>>> + adjust_end(cd); >>>> + if (wrapped && cd->end_of_data != cd->end_addr) >>>> + cd->cur_ptr = cd->end_of_data + 1; >>>> + else >>>> + cd->cur_ptr = cd->start_addr; >>>> + >>>> + fp->private_data = cd; >>>> + >>>> + return 0; >>>> + >>>> +err_magic: >>>> + iounmap(cd->map_addr); >>>> + >>>> +err_ioremap: >>>> + kfree(cd); >>>> + >>>> + return err; >>>> +} >>>> + >>>> +static int dpaa2_mc_console_open(struct inode *node, struct file >>>> +*fp) { >>>> + return dpaa2_generic_console_open(node, fp, >>>> + MC_BUFFER_OFFSET, MC_BUFFER_SIZE, >>>> + MAGIC_MC, MC_OFFSET_DELTA); >>>> +} >>>> + >>>> +static int dpaa2_aiop_console_open(struct inode *node, struct file >>>> +*fp) { >>>> + return dpaa2_generic_console_open(node, fp, >>>> + AIOP_BUFFER_OFFSET, >>>> AIOP_BUFFER_SIZE, >>>> + MAGIC_AIOP, >>>> +AIOP_OFFSET_DELTA); } >>>> + >>>> +static int dpaa2_console_close(struct inode *node, struct file *fp) >>>> +{ >>>> + struct console_data *cd = fp->private_data; >>>> + >>>> + iounmap(cd->map_addr); >>>> + kfree(cd); >>>> + return 0; >>>> +} >>>> + >>>> +static ssize_t dpaa2_console_read(struct file *fp, char __user *buf, >>>> + size_t count, loff_t *f_pos) { >>>> + struct console_data *cd = fp->private_data; >>>> + size_t bytes = dpaa2_console_size(cd); >>>> + size_t bytes_end = cd->end_addr - cd->cur_ptr; >>>> + size_t written = 0; >>>> + >>>> + /* Check if we need to adjust the end of data addr */ >>>> + adjust_end(cd); >>>> + >>>> + if (cd->end_of_data == cd->cur_ptr) >>>> + return 0; >>>> + >>>> + if (count < bytes) >>>> + bytes = count; >>>> + >>>> + if (bytes > bytes_end) { >>>> + if (copy_to_user(buf, cd->cur_ptr, bytes_end)) >>>> + return -EFAULT; >>>> + buf += bytes_end; >>>> + cd->cur_ptr = cd->start_addr; >>>> + bytes -= bytes_end; >>>> + written += bytes_end; >>>> + } >>>> + >>>> + if (copy_to_user(buf, cd->cur_ptr, bytes)) >>>> + return -EFAULT; >>>> + cd->cur_ptr += bytes; >>>> + written += bytes; >>>> + >>>> + return written; >>>> +} >>>> + >>>> +static const struct file_operations dpaa2_mc_console_fops = { >>>> + .owner = THIS_MODULE, >>>> + .open = dpaa2_mc_console_open, >>>> + .release = dpaa2_console_close, >>>> + .read = dpaa2_console_read, >>>> +}; >>>> + >>>> +static struct miscdevice dpaa2_mc_console_dev = { >>>> + .minor = MISC_DYNAMIC_MINOR, >>>> + .name = "dpaa2_mc_console", >>>> + .fops = &dpaa2_mc_console_fops }; >>>> + >>>> +static const struct file_operations dpaa2_aiop_console_fops = { >>>> + .owner = THIS_MODULE, >>>> + .open = dpaa2_aiop_console_open, >>>> + .release = dpaa2_console_close, >>>> + .read = dpaa2_console_read, >>>> +}; >>>> + >>>> +static struct miscdevice dpaa2_aiop_console_dev = { >>>> + .minor = MISC_DYNAMIC_MINOR, >>>> + .name = "dpaa2_aiop_console", >>>> + .fops = &dpaa2_aiop_console_fops }; >>>> + >>>> +static int dpaa2_console_probe(struct platform_device *pdev) { >>>> + int error; >>>> + >>>> + error = of_address_to_resource(pdev->dev.of_node, 0, >> &mc_base_addr); >>>> + if (error < 0) { >>>> + pr_err("of_address_to_resource() failed for %pOF with >>>> %d\n", >>>> + pdev->dev.of_node, error); >>>> + return error; >>>> + } >>>> + >>>> + error = misc_register(&dpaa2_mc_console_dev); >>>> + if (error) { >>>> + pr_err("dpaa2-console: cannot register device %s\n", >>>> + dpaa2_mc_console_dev.name); >>> >>> The pr_fmt already have the dpaa2-console prefix. >>> >>>> + goto err_register_mc; >>>> + } >>>> + >>>> + error = misc_register(&dpaa2_aiop_console_dev); >>>> + if (error) { >>>> + pr_err("dpaa2-console: cannot register device %s\n", >>>> + dpaa2_aiop_console_dev.name); >>> >>> The pr_fmt already have the dpaa2-console prefix. >>> >> >> >> Yep, will remove the duplicate "dpaa2-console" prefix. >> >> Thanks for the review, >> Ioana C >> >> >>>> + goto err_register_aiop; >>>> + } >>>> + >>>> + return 0; >>>> + >>>> +err_register_aiop: >>>> + misc_deregister(&dpaa2_mc_console_dev); >>>> +err_register_mc: >>>> + return error; >>>> +} >>>> + >>>> +static int dpaa2_console_remove(struct platform_device *pdev) { >>>> + misc_deregister(&dpaa2_mc_console_dev); >>>> + misc_deregister(&dpaa2_aiop_console_dev); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct of_device_id dpaa2_console_match_table[] = { >>>> + { .compatible = "fsl,dpaa2-console",}, >>>> + {}, >>>> +}; >>>> + >>>> +MODULE_DEVICE_TABLE(of, dpaa2_console_match_table); >>>> + >>>> +static struct platform_driver dpaa2_console_driver = { >>>> + .driver = { >>>> + .name = "dpaa2-console", >>>> + .pm = NULL, >>>> + .of_match_table = dpaa2_console_match_table, >>>> + }, >>>> + .probe = dpaa2_console_probe, >>>> + .remove = dpaa2_console_remove, }; >>>> +module_platform_driver(dpaa2_console_driver); >>>> -- >>>> 1.9.1 >>>> >>> > >