Messages in this thread Patch in this message |  | | From | Leo Li <> | Subject | RE: [PATCH] soc: fsl: add DPAA2 console support | Date | Thu, 4 Apr 2019 18:20:00 +0000 |
| |
> -----Original Message----- > From: Ioana Ciornei > Sent: Thursday, April 4, 2019 5:48 AM > To: Leo Li <leoyang.li@nxp.com> > Cc: linux-kernel@vger.kernel.org; Roy Pledge <roy.pledge@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.ciornei@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.ciornei@nxp.com> > >> Signed-off-by: Roy Pledge <roy.pledge@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?
> > >> + 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.pledge@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 > >> > >
|  |