Re: [PATCH] soc: fsl: add DPAA2 console support

From: Ioana Ciornei
Date: Fri Apr 05 2019 - 05:05:35 EST

Next message: Ludovic Desroches: "Re: linux-next: manual merge of the at91 tree with the at91-fixes tree"
Previous message: Julien Grall: "Re: [RFC PATCH] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state"
In reply to: Leo Li: "RE: [PATCH] soc: fsl: add DPAA2 console support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.li@xxxxxxx>
>> Cc: linux-kernel@xxxxxxxxxxxxxxx; Roy Pledge <roy.pledge@xxxxxxx>
>> 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@xxxxxxx>
>> 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@xxxxxxx>
>>>> Signed-off-by: Roy Pledge <roy.pledge@xxxxxxx>
>>>> ---
>>>> 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.pledge@xxxxxxx>");
>>>> +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
>>>>
>>>
>
>

Next message: Ludovic Desroches: "Re: linux-next: manual merge of the at91 tree with the at91-fixes tree"
Previous message: Julien Grall: "Re: [RFC PATCH] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state"
In reply to: Leo Li: "RE: [PATCH] soc: fsl: add DPAA2 console support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]