* Re: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Jerry-ch Chen @ 2019-08-25 9:18 UTC (permalink / raw)
To: Tomasz Figa
Cc: devicetree@vger.kernel.org, Sean Cheng (鄭昇弘),
Frederic Chen (陳俊元),
Rynn Wu (吳育恩), srv_heupstream, po-yang.huang,
suleiman@chromium.org, shik@chromium.org, jerry-ch.chen,
Jungo Lin (林明俊),
Sj Huang (黃信璋), yuzhao@chromium.org,
linux-mediatek@lists.infradead.org, zwisler@chromium.org,
hans.verkuil@cisco.com, matthias.bgg@gmail.com,
Christie Yu (游雅惠), mchehab@kernel.org,
laurent.pinchart+renesas@ideasonboard.com,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <20190802082815.GA203993@chromium.org>
Hi Tomasz,
On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote:
> Hi Jerry,
>
> On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote:
> > From: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> >
> > This patch adds the driver of Face Detection (FD) unit in
> > Mediatek camera system, providing face detection function.
> >
> > The mtk-isp directory will contain drivers for multiple IP
> > blocks found in Mediatek ISP system. It will include ISP Pass 1
> > driver (CAM), sensor interface driver, DIP driver and face
> > detection driver.
> >
> > Signed-off-by: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> > ---
> > drivers/media/platform/Makefile | 2 +
> > drivers/media/platform/mtk-isp/fd/Makefile | 5 +
> > drivers/media/platform/mtk-isp/fd/mtk_fd.h | 157 +++
> > drivers/media/platform/mtk-isp/fd/mtk_fd_40.c | 1259 +++++++++++++++++++++++++
> > include/uapi/linux/v4l2-controls.h | 4 +
> > 5 files changed, 1427 insertions(+)
> > create mode 100644 drivers/media/platform/mtk-isp/fd/Makefile
> > create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> >
>
> Thanks for the patch! I finally got a chance to fully review the code. Sorry
> for the delay. Please check my comments inline.
>
I appreciate your comments.
I've fixed most of the comments and verifying them,
Sorry for the delay, here is the reply.
>
> > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > index e6deb25..8b817cc 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -94,6 +94,8 @@ obj-$(CONFIG_VIDEO_MEDIATEK_MDP) += mtk-mdp/
> >
> > obj-$(CONFIG_VIDEO_MEDIATEK_JPEG) += mtk-jpeg/
> >
> > +obj-$(CONFIG_VIDEO_MEDIATEK_FD) += mtk-isp/fd/
> > +
> > obj-$(CONFIG_VIDEO_QCOM_CAMSS) += qcom/camss/
> >
> > obj-$(CONFIG_VIDEO_QCOM_VENUS) += qcom/venus/
> > diff --git a/drivers/media/platform/mtk-isp/fd/Makefile b/drivers/media/platform/mtk-isp/fd/Makefile
> > new file mode 100644
> > index 0000000..9b1c501
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +mtk-fd-objs += mtk_fd_40.o
> > +
> > +obj-$(CONFIG_VIDEO_MEDIATEK_FD) += mtk-fd.o
> > \ No newline at end of file
> > diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd.h b/drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > new file mode 100644
> > index 0000000..289999b
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > @@ -0,0 +1,157 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +#ifndef __MTK_FD_HW_H__
> > +#define __MTK_FD_HW_H__
> > +
> > +#include <linux/io.h>
> > +#include <linux/types.h>
> > +#include <linux/platform_device.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/videobuf2-v4l2.h>
> > +
> > +#define MTK_FD_OUTPUT_MIN_WIDTH 26U
> > +#define MTK_FD_OUTPUT_MIN_HEIGHT 26U
> > +#define MTK_FD_OUTPUT_MAX_WIDTH 640U
> > +#define MTK_FD_OUTPUT_MAX_HEIGHT 480U
> > +
> > +/* Control the user defined image widths and heights
> > + * to be scaled and performed face detection in FD HW.
> > + * MTK FD support up to 14 user defined image sizes to perform face detection.
> > + */
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_WIDTH (V4L2_CID_USER_MTK_FD_BASE + 1)
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT (V4L2_CID_USER_MTK_FD_BASE + 2)
> > +
> > +/* Control the numbers of user defined image sizes.
> > + * The default value is 0 which means user is not going
> > + * to define the specific image sizes.
> > + */
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_NUM (V4L2_CID_USER_MTK_FD_BASE + 3)
> > +
> > +/* Control the Face Pose to be detected.
> > + * Here describe the value as following:
> > + * {0, detect the front face with rotation from 0 to 270 degrees},
> > + * {1, detect the front face with rotation from 0 to 240 and 300 degrees},
> > + * {2, detect the front face with rotation from 0 to 240 and 330 degrees},
> > + * {3, detect the front face with rotation from 0 to 240 and left side face}.
> > + */
> > +#define V4L2_CID_MTK_FD_DETECT_POSE (V4L2_CID_USER_MTK_FD_BASE + 4)
> > +#define V4L2_CID_MTK_FD_DETECT_SPEEDUP (V4L2_CID_USER_MTK_FD_BASE + 5)
> > +#define V4L2_CID_MTK_FD_EXTRA_MODEL (V4L2_CID_USER_MTK_FD_BASE + 6)
> > +
> > +/* We reserve 16 controls for this driver. */
> > +#define V4L2_CID_MTK_FD_MAX 16
> > +
> > +#define ENABLE_FD 0x111
> > +#define FD_HW_ENABLE 0x4
> > +#define FD_INT_EN 0x15c
> > +#define FD_INT 0x168
> > +#define FD_RESULT 0x178
> > +#define FD_IRQ_MASK 0x001
> > +
> > +#define RS_MAX_BUF_SIZE 2288788
> > +#define FD_MAX_SPEEDUP 7
> > +#define FD_MAX_POSE_VAL 0xfffffffffffffff
> > +#define FD_DEF_POSE_VAL 0x3ff
> > +#define MAX_FD_SEL_NUM 1026
> > +
> > +/* The max. number of face sizes could be detected, for feature scaling */
> > +#define FACE_SIZE_NUM_MAX 14
> > +/* FACE_SIZE_NUM_MAX + 1, first scale for input image W/H */
> > +#define FD_SCALE_NUM 15
> > +
> > +enum fd_state {
> > + FD_ENQ,
> > + FD_CBD,
> > +};
> > +
> > +enum fd_img_format {
> > + FMT_VYUY = 2,
> > + FMT_UYVY,
> > + FMT_YVYU,
> > + FMT_YUYV,
> > + FMT_UNKNOWN
> > +};
>
> Please use #define macros for hardware/firmware interface definitions.
>
Ok, I will refine as following
#define MTK_FD_HW_FMT_VYUY 2
#define MTK_FD_HW_FMT_UYVY 3
#define MTK_FD_HW_FMT_YVYU 4
#define MTK_FD_HW_FMT_YUYV 5
#define MTK_FD_HW_FMT_UNKNOWN 6
> > +
> > +struct fd_buffer {
> > + __u32 scp_addr; /* used by SCP */
> > + __u32 dma_addr; /* used by DMA HW */
> > +} __packed;
> > +
> > +enum fd_scp_cmd {
> > + FD_CMD_INIT,
> > + FD_CMD_ENQUEUE,
> > +};
>
> Ditto.
Ok, I will refine as following
#define MTK_FD_IPI_CMD_INIT 0
#define MTK_FD_IPI_CMD_ENQUEUE 1
>
> > +
> > +struct fd_user_output {
> > + __u64 results[MAX_FD_SEL_NUM];
> > + __u16 number;
> > +};
> > +
> > +struct user_param {
> > + u8 fd_pose;
> > + u8 fd_speedup;
> > + u8 fd_extra_model;
> > + u8 scale_img_num;
> > + u8 src_img_fmt;
> > + __u16 scale_img_width[FD_SCALE_NUM];
> > + __u16 scale_img_height[FD_SCALE_NUM];
> > +} __packed;
> > +
> > +struct fd_hw_param {
> > + struct fd_buffer src_img;
> > + struct fd_buffer user_result;
> > + struct user_param user_param;
> > +} __packed;
> > +
> > +struct cmd_init_info {
> > + struct fd_buffer fd_manager;
> > + __u32 rs_dma_addr;
> > +} __packed;
> > +
> > +struct ipi_message {
> > + u8 cmd_id;
> > + union {
> > + struct cmd_init_info init_param;
> > + struct fd_hw_param hw_param;
> > + };
> > +} __packed;
> > +
> > +struct mtk_fd_hw {
> > + struct clk *fd_clk;
> > + struct rproc *rproc_handle;
> > + struct platform_device *scp_pdev;
> > + struct fd_buffer scp_mem;
> > + wait_queue_head_t wq;
> > + void __iomem *fd_base;
> > + atomic_t fd_user_cnt;
> > + enum fd_state state;
> > + u32 fd_irq_result;
> > + /* Ensure only one job in hw */
> > + struct mutex fd_hw_lock;
> > +};
> > +
> > +struct mtk_fd_dev {
> > + struct platform_device *pdev;
> > + struct v4l2_device v4l2_dev;
> > + struct v4l2_m2m_dev *m2m_dev;
> > + struct media_device mdev;
> > + struct video_device vfd;
> > + struct mtk_fd_hw fd_hw;
>
> Could we just put the contents of that struct directly here? That should
> simplify dereference chains in the code.
>
Ok, I will fix it.
> > + /* Lock for V4L2 operations */
> > + struct mutex vfd_lock;
> > +};
> > +
> > +struct mtk_fd_ctx {
> > + struct mtk_fd_dev *fd_dev;
> > + struct device *dev;
> > + struct v4l2_fh fh;
> > + struct v4l2_ctrl_handler hdl;
> > + struct v4l2_pix_format_mplane src_fmt;
> > + struct v4l2_meta_format dst_fmt;
> > + struct user_param user_param;
> > +};
> > +
> > +#endif/*__MTK_FD_HW_H__*/
> > diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c b/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> > new file mode 100644
> > index 0000000..246d3aa
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> > @@ -0,0 +1,1259 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +#include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_data/mtk_scp.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/remoteproc.h>
> > +#include <media/videobuf2-dma-contig.h>
> > +#include <media/videobuf2-v4l2.h>
> > +#include <linux/wait.h>
> > +#include <media/v4l2-ioctl.h>
> > +#include <media/v4l2-event.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-mem2mem.h>
> > +#include <media/videobuf2-core.h>
> > +
> > +#include "mtk_fd.h"
> > +
> > +static struct v4l2_meta_format fw_param_fmts[] = {
>
> const?
>
> Also, isn't this the buffer with the detected faces rather than params?
This struct will be removed. Yes, it's the buffer with detected faces.
>
> > + {
> > + .dataformat = V4L2_META_FMT_MTISP_PARAMS,
>
> This should use its own format.
Ok, I will define V4L2_META_FMT_MTFD_RESULT in videodev2.h
>
> > + .buffersize = 1024 * 30,
>
> Please define a C struct for this buffer and use sizeof() here.
>
Ok, that will be sizeof(struct fd_user_output);
> > + },
> > +};
>
> Actually, is there a reason to have this array at all if there is only 1
> format supported? I think we could just put the right constants directly in
> the code.
>
I agree, I'll remove this array and use the constants in the code.
> > +
> > +static const struct v4l2_pix_format_mplane in_img_fmts[] = {
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_VYUY,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
> > + },
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_YUYV,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
> > + },
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_YVYU,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
> > + },
> > + {
> > + .width = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_UYVY,
> > + .colorspace = V4L2_COLORSPACE_BT2020,
> > + .field = V4L2_FIELD_NONE,
> > + .num_planes = 1,
>
> If all the formats have the same values for a field, there is no reason to
> have the field initialized here. Just make initialize them to the constant
> values inside TRY_FMT.
>
Ok, I will do so.
> Hmm, are the interleaved YUV formats the only formats supported by this
> hardware? That would be quite unfortunate given the formats supported by the
> other IP blocks on this SoC use the more typical planar formats.
>
The supported YUV 1 plane formats are listed above.
The rest supported format is YUV 2 plane format, which should be
V4L2_PIX_FMT_NV16M.
For the in_img_fmts[], I will refine as following:
static const struct v4l2_pix_format_mplane mtk_fd_img_fmts[] = {
{
.pixelformat = V4L2_PIX_FMT_VYUY,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_YUYV,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_YVYU,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_UYVY,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_NV16M,
.num_planes = 2,
},
{
.pixelformat = V4L2_PIX_FMT_NV61M,
.num_planes = 2,
},
};
> > + },
> > +};
> > +
> > +#define NUM_FORMATS ARRAY_SIZE(in_img_fmts)
> > +
> > +static inline struct mtk_fd_dev *mtk_fd_hw_to_dev(struct mtk_fd_hw *fd_hw)
> > +{
> > + return container_of(fd_hw, struct mtk_fd_dev, fd_hw);
> > +}
> > +
> > +static inline struct mtk_fd_ctx *fh_to_ctx(struct v4l2_fh *fh)
> > +{
> > + return container_of(fh, struct mtk_fd_ctx, fh);
> > +}
> > +
> > +static int mtk_fd_load_scp(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > + phandle rproc_phandle;
> > + int ret;
> > +
> > + /* init scp */
> > + fd_hw->scp_pdev = scp_get_pdev(fd_dev->pdev);
> > + if (!fd_hw->scp_pdev) {
> > + dev_err(dev, "Failed to get scp device\n");
> > + return -ENODEV;
> > + }
> > +
> > + if (of_property_read_u32(fd_dev->pdev->dev.of_node, "mediatek,scp",
> > + &rproc_phandle)) {
> > + dev_err(dev, "Could not get scp device\n");
> > + return -EINVAL;
> > + }
> > +
> > + fd_hw->rproc_handle = rproc_get_by_phandle(rproc_phandle);
> > + if (!fd_hw->rproc_handle) {
> > + dev_err(dev, "Could not get FD's rproc_handle\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = rproc_boot(fd_hw->rproc_handle);
> > + if (ret < 0) {
> > + /**
> > + * Return 0 if downloading firmware successfully,
> > + * otherwise it is failed
> > + */
> > + dev_err(dev, "rproc_boot failed\n");
> > + return -ENODEV;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static dma_addr_t mtk_fd_hw_alloc_rs_dma_addr(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > + void *va;
> > + dma_addr_t dma_handle;
> > +
> > + va = dma_alloc_coherent(dev, RS_MAX_BUF_SIZE, &dma_handle, GFP_KERNEL);
>
> I see this allocated here, but I don't see this freed anywhere.
>
I will fix it in next version.
> > + if (!va) {
> > + dev_err(dev, "dma_alloc null va\n");
> > + return -ENOMEM;
> > + }
> > + memset(va, 0, RS_MAX_BUF_SIZE);
> > +
> > + return dma_handle;
> > +}
> > +
> > +static int mtk_fd_send_ipi_init(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct ipi_message fd_init_msg;
> > + dma_addr_t rs_dma_addr;
> > +
> > + fd_init_msg.cmd_id = FD_CMD_INIT;
> > +
> > + fd_init_msg.init_param.fd_manager.scp_addr = fd_hw->scp_mem.scp_addr;
> > + fd_init_msg.init_param.fd_manager.dma_addr = fd_hw->scp_mem.dma_addr;
> > +
> > + rs_dma_addr = mtk_fd_hw_alloc_rs_dma_addr(fd_hw);
> > + if (!rs_dma_addr)
> > + return -ENOMEM;
> > +
> > + fd_init_msg.init_param.rs_dma_addr = rs_dma_addr;
> > +
> > + return scp_ipi_send(fd_hw->scp_pdev, SCP_IPI_FD_CMD, &fd_init_msg,
> > + sizeof(fd_init_msg), 0);
> > +}
> > +
> > +static int mtk_fd_hw_enable(struct mtk_fd_hw *fd_hw)
> > +{
> > + int ret;
> > +
> > + ret = mtk_fd_load_scp(fd_hw);
> > + if (ret)
> > + return ret;
> > +
> > + ret = mtk_fd_send_ipi_init(fd_hw);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_hw_connect(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + s32 usercount;
> > +
> > + mutex_lock(&fd_hw->fd_hw_lock);
>
> First of all, we don't need a mutex here, because all the V4L2 ioctls are
> already running with the video device mutex.
Ok, I will remove the using of the mutex here.
>
> > + usercount = atomic_inc_return(&fd_hw->fd_user_cnt);
>
> If we already use a mutex, there is no point in using atomic types.
>
Ok, I will replace the atomic types with normal types
> > + if (usercount == 1) {
> > + pm_runtime_get_sync(&fd_dev->pdev->dev);
> > + if (mtk_fd_hw_enable(fd_hw)) {
> > + pm_runtime_put_sync(&fd_dev->pdev->dev);
> > + atomic_dec_return(&fd_hw->fd_user_cnt);
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > + return -EINVAL;
> > + }
> > + }
>
> This is a simple mem-to-mem device, so there is no reason to keep it active
> all the time it's streaming. Please just get the runtime PM counter when
> queuing a job to the hardware and release it when the job finishes.
>
> I guess we might still want to do the costly operations like rproc_boot()
> when we start streaming, though.
>
Do you mean by moving the pm_runtime_get/put stuff to the job execution
and job finish place?
the rproc_boot() operation will be done here.
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_wait_irq(struct mtk_fd_hw *fd_hw)
> > +{
> > + int timeout;
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > +
> > + timeout = wait_event_interruptible_timeout
> > + (fd_hw->wq, (fd_hw->fd_irq_result & FD_IRQ_MASK),
> > + usecs_to_jiffies(1000000));
> > + if (!timeout) {
> > + dev_err(dev, "%s timeout, %d\n", __func__,
> > + fd_hw->fd_irq_result);
> > + return -EAGAIN;
> > + } else if (!(fd_hw->fd_irq_result & FD_IRQ_MASK)) {
> > + dev_err(dev, "%s No IRQ mask:0x%8x\n",
> > + __func__, fd_hw->fd_irq_result);
> > + return -EINVAL;
> > + }
> > + fd_hw->fd_irq_result = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_fd_hw_disconnect(struct mtk_fd_hw *fd_hw)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + s32 usercount;
> > +
> > + mutex_lock(&fd_hw->fd_hw_lock);
> > + atomic_dec_return(&fd_hw->fd_user_cnt);
> > + usercount = atomic_read(&fd_hw->fd_user_cnt);
> > + if (usercount == 0) {
> > + if (fd_hw->state == FD_ENQ)
> > + mtk_fd_wait_irq(fd_hw);
>
> This shouldn't be possible to happen as the queues should be already stopped
> at this point.
>
Ok, I will remove it.
> > +
> > + pm_runtime_put_sync(&fd_dev->pdev->dev);
>
> Any reason to use pm_runtime_put_sync() over pm_runtime_put()?
>
No special reason to do so, the pm_runtime_put_sync here will be moved
to the place each job finished.
> > + rproc_shutdown(fd_hw->rproc_handle);
> > + rproc_put(fd_hw->rproc_handle);
> > + }
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +}
> > +
> > +static void mtk_fd_hw_job_finish(struct mtk_fd_hw *fd_hw,
> > + struct fd_hw_param *fd_param,
> > + enum vb2_buffer_state vb_state)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct mtk_fd_ctx *ctx;
> > + struct device *dev = &fd_dev->pdev->dev;
> > + struct vb2_buffer *src_vb, *dst_vb;
> > + struct vb2_v4l2_buffer *src_vbuf = NULL, *dst_vbuf = NULL;
> > +
> > + ctx = v4l2_m2m_get_curr_priv(fd_dev->m2m_dev);
> > + if (!ctx) {
> > + dev_err(dev, "Instance released before end of transaction\n");
> > + return;
> > + }
>
> This shouldn't be possible. I suspect you're seeing this because
> .stop_streaming is not implemented correctly. See my comment there.
>
Ok, I will remove this.
> > +
> > + src_vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + if (WARN_ON(!src_vb))
> > + return;
>
> This shouldn't be possible.
>
Ditto.
> > + src_vbuf = to_vb2_v4l2_buffer(src_vb);
> > +
> > + dst_vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > + if (WARN_ON(!dst_vb))
> > + return;
>
> Ditto.
>
Ditto.
> > + dst_vbuf = to_vb2_v4l2_buffer(dst_vb);
> > +
> > + dst_vbuf->vb2_buf.timestamp = src_vbuf->vb2_buf.timestamp;
> > + dst_vbuf->timecode = src_vbuf->timecode;
> > + dst_vbuf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> > + dst_vbuf->flags |= src_vbuf->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>
> We should be able to just use v4l2_m2m_buf_copy_metadata().
>
Ok, I will refine as:
v4l2_m2m_buf_copy_metadata(src_vbuf, dst_vbuf,
V4L2_BUF_FLAG_TSTAMP_SRC_MASK);
> > +
> > + v4l2_m2m_buf_done(src_vbuf, vb_state);
> > + v4l2_m2m_buf_done(dst_vbuf, vb_state);
> > + v4l2_m2m_job_finish(fd_dev->m2m_dev, ctx->fh.m2m_ctx);
> > +}
> > +
> > +static int mtk_fd_hw_job_exec(struct mtk_fd_hw *fd_hw,
> > + struct fd_hw_param *fd_param,
> > + void *output_vaddr)
> > +{
> > + struct fd_user_output *fd_output;
> > + struct ipi_message fd_ipi_msg;
> > + int ret;
> > + u32 num;
> > +
> > + if (fd_param->user_param.src_img_fmt == FMT_UNKNOWN)
> > + goto param_err;
>
> Is this possible?
>
Only if user set wrong format, I will remove this.
> > +
> > + mutex_lock(&fd_hw->fd_hw_lock);
> > + fd_hw->state = FD_ENQ;
>
> What is this state for?
>
It was for checking status, if a job is processing, the state is
FD_ENQ,
then we should wait for the last job to be done when pm_suspend().
> > + fd_ipi_msg.cmd_id = FD_CMD_ENQUEUE;
> > + memcpy(&fd_ipi_msg.hw_param, fd_param, sizeof(fd_ipi_msg.hw_param));
> > + ret = scp_ipi_send(fd_hw->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg,
> > + sizeof(fd_ipi_msg), 0);
> > + if (ret)
> > + goto buf_err;
> > +
> > + ret = mtk_fd_wait_irq(fd_hw);
> > + if (ret)
> > + goto buf_err;
>
> This function is called from device_run and it shouldn't be synchronous. It
> should only queue the job to the hardware to be handled asynchronously when
> it finishes.
>
Ok, I will fix it.
> > +
> > + num = readl(fd_hw->fd_base + FD_RESULT);
> > + /* Disable FD ISR */
> > + writel(0x0, fd_hw->fd_base + FD_INT_EN);
> > +
> > + fd_output = (struct fd_user_output *)output_vaddr;
> > + fd_output->number = num;
> > + fd_hw->state = FD_CBD;
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +
> > + mtk_fd_hw_job_finish(fd_hw, fd_param, VB2_BUF_STATE_DONE);
> > + return 0;
> > +
> > +buf_err:
> > + mutex_unlock(&fd_hw->fd_hw_lock);
> > +param_err:
> > + mtk_fd_hw_job_finish(fd_hw, fd_param, VB2_BUF_STATE_ERROR);
> > + return ret;
> > +}
> > +
> > +static int mtk_fd_vb2_buf_out_validate(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> > +
> > + v4l2_buf->field = V4L2_FIELD_NONE;
>
> We need to fail with -EINVAL if v4l2_buf->field was different than
> V4L2_FIELD_ANY or _NONE.
>
Ok, I will refine as following:
if (v4l2_buf->field == V4L2_FIELD_ANY)
v4l2_buf->field = V4L2_FIELD_NONE;
if (v4l2_buf->field != V4L2_FIELD_NONE)
return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_vb2_buf_prepare(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > + struct vb2_queue *vq = vb->vb2_queue;
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct device *dev = ctx->dev;
> > + struct v4l2_pix_format_mplane *pixfmt;
> > +
> > + switch (vq->type) {
> > + case V4L2_BUF_TYPE_META_CAPTURE:
> > + if (vb2_plane_size(vb, 0) < ctx->dst_fmt.buffersize) {
> > + dev_err(dev, "meta size %d is too small\n");
>
> Please make this dev_dbg(), because userspace misbehavior must not trigger
> kernel error logs.
>
Ok, fixed.
> > + return -EINVAL;
> > + }
> > + break;
> > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > + pixfmt = &ctx->src_fmt;
> > +
> > + if (vbuf->field == V4L2_FIELD_ANY)
> > + vbuf->field = V4L2_FIELD_NONE;
> > +
> > + if (vb->num_planes != 1 || vbuf->field != V4L2_FIELD_NONE) {
> > + dev_err(dev, "plane or field %d not supported\n",
> > + vb->num_planes, vbuf->field);
>
> Ditto.
>
Done.
> > + return -EINVAL;
> > + }
> > + if (vb2_plane_size(vb, 0) < pixfmt->plane_fmt[0].sizeimage) {
> > + dev_err(dev, "plane %d is too small\n");
>
> Ditto.
>
Done.
> > + return -EINVAL;
> > + }
> > + break;
> > + default:
> > + dev_err(dev, "invalid queue type: %d\n", vq->type);
> > + return -EINVAL;
>
> We shouldn't need to handle this.
>
Ok, I will remove this.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_fd_vb2_buf_queue(struct vb2_buffer *vb)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +
> > + v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> > +}
> > +
> > +static int mtk_fd_vb2_queue_setup(struct vb2_queue *vq,
> > + unsigned int *num_buffers,
> > + unsigned int *num_planes,
> > + unsigned int sizes[],
> > + struct device *alloc_devs[])
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct device *dev = ctx->dev;
> > + unsigned int size;
> > +
> > + switch (vq->type) {
> > + case V4L2_BUF_TYPE_META_CAPTURE:
> > + size = ctx->dst_fmt.buffersize;
> > + break;
> > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > + size = ctx->src_fmt.plane_fmt[0].sizeimage;
> > + break;
> > + default:
> > + dev_err(dev, "invalid queue type: %d\n", vq->type);
>
> We should need to handle this.
>
Do you mean by giving a size instead of return -EINVAL?
> > + return -EINVAL;
> > + }
> > +
> > + if (!*num_planes) {
> > + *num_planes = 1;
> > + sizes[0] = size;
> > + }
>
> We need to handle the case when *num_planes is non-zero. We then need to
> check if it's 1 and *sizes >= size and fail with -EINVAL if not.
>
Ok, I will refine as following:
if (*num_planes) {
if (sizes[0] < size || *num_planes != 1)
return -EINVAL;
} else {
*num_planes = 1;
sizes[0] = size;
}
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_vb2_start_streaming(struct vb2_queue *vq, unsigned int count)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > +
> > + return mtk_fd_hw_connect(&ctx->fd_dev->fd_hw);
>
> This would be called twice for every context, once for the VIDEO_OUTPUT
> queue and once for the META_CAPTURE queue. Perhaps it would make sense to
> just do this for the VIDEO_OUTPUT queue?
>
Ok, I will refine as:
if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
return mtk_fd_hw_connect(&ctx->fd_dev->fd_hw);
else
return 0;
> > +}
> > +
> > +static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct vb2_buffer *vb;
>
> How do we guarantee here that the hardware isn't still accessing the buffers
> removed below?
>
Maybe we can check the driver state flag and aborting the unfinished
jobs?
(fd_hw->state == FD_ENQ)
> > +
> > + if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > + vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + else
> > + vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > +
> > + while (vb) {
> > + v4l2_m2m_buf_done(to_vb2_v4l2_buffer(vb), VB2_BUF_STATE_ERROR);
> > + if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > + vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > + else
> > + vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > + }
>
> We can use v4l2_m2m_buf_remove(). Also we can move the call into the loop
> condition:
>
> while ((vb == v4l2_m2m_buf_remove(...)))
> v4l2_m2m_buf_done(...);
>
Ok, I will refine as following:
while ((vb = v4l2_m2m_buf_remove(V4L2_TYPE_IS_OUTPUT(vq->type)?
&m2m_ctx->out_q_ctx :
&m2m_ctx->cap_q_ctx)))
v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
> > +
> > + mtk_fd_hw_disconnect(&ctx->fd_dev->fd_hw);
>
> This should also probably be done only for 1 queue. Since the VIDEO_OUTPUT
> queue supports requestes, I'd consider it the master one.
>
Ok, only for output queue to trigger disconnect.
if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
mtk_fd_hw_disconnect(&ctx->fd_dev->fd_hw);
> > +}
> > +
> > +static void mtk_fd_vb2_request_complete(struct vb2_buffer *vb)
> > +{
> > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +
> > + v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
> > +}
> > +
> > +static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
> > + const struct v4l2_pix_format_mplane *sfmt)
> > +{
> > + dfmt->width = sfmt->width;
> > + dfmt->height = sfmt->height;
> > + dfmt->pixelformat = sfmt->pixelformat;
> > + dfmt->field = sfmt->field;
> > + dfmt->colorspace = sfmt->colorspace;
> > + dfmt->num_planes = sfmt->num_planes;
> > +
> > + /* Use default */
> > + dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > + dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > + dfmt->xfer_func =
> > + V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
> > + dfmt->plane_fmt[0].bytesperline = dfmt->width * 2;
> > + dfmt->plane_fmt[0].sizeimage =
> > + dfmt->height * dfmt->plane_fmt[0].bytesperline;
> > + memset(dfmt->reserved, 0, sizeof(dfmt->reserved));
> > +}
>
> Could we unify this function with mtk_fd_m2m_try_fmt_out_mp()? That function
> should be almost directly reusable for the default format initialization +/-
> the arguments passed to it.
>
Ok, I will try to reuse it as following:
static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
const struct v4l2_pix_format_mplane *sfmt)
{
dfmt->field = V4L2_FIELD_NONE;
dfmt->colorspace = V4L2_COLORSPACE_BT2020;
dfmt->num_planes = sfmt->num_planes;
dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
dfmt->xfer_func =
V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
/* Keep user setting as possible */
dfmt->width = clamp(dfmt->width,
MTK_FD_OUTPUT_MIN_WIDTH,
MTK_FD_OUTPUT_MAX_WIDTH);
dfmt->height = clamp(dfmt->height,
MTK_FD_OUTPUT_MIN_HEIGHT,
MTK_FD_OUTPUT_MAX_HEIGHT);
if (sfmt->num_planes == 2) {
/* NV16M and NV61M has 1 byte per pixel */
dfmt->plane_fmt[0].bytesperline = dfmt->width;
dfmt->plane_fmt[1].bytesperline = dfmt->width;
} else {
/* 2 bytes per pixel */
dfmt->plane_fmt[0].bytesperline = dfmt->width * 2;
}
dfmt->plane_fmt[0].sizeimage =
dfmt->height * dfmt->plane_fmt[0].bytesperline;
}
> > +
> > +static const struct v4l2_pix_format_mplane *mtk_fd_find_fmt(u32 format)
> > +{
> > + unsigned int i;
> > + const struct v4l2_pix_format_mplane *dev_fmt;
> > +
> > + for (i = 0; i < NUM_FORMATS; i++) {
> > + dev_fmt = &in_img_fmts[i];
> > + if (dev_fmt->pixelformat == format)
> > + return dev_fmt;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static int mtk_fd_m2m_querycap(struct file *file, void *fh,
> > + struct v4l2_capability *cap)
> > +{
> > + struct mtk_fd_dev *fd_dev = video_drvdata(file);
> > + struct device *dev = &fd_dev->pdev->dev;
> > +
> > + strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
>
> Please use dev_driver_string().
>
Done,
> > + strscpy(cap->card, fd_dev->vfd.name, sizeof(cap->card));
>
> I think we can also make this dev_driver_string().
>
Done.
> > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> > + dev_name(&fd_dev->pdev->dev));
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_enum_fmt_out_mp(struct file *file, void *fh,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < NUM_FORMATS; ++i) {
> > + if (i == f->index) {
> > + f->pixelformat = in_img_fmts[i].pixelformat;
> > + return 0;
> > + }
> > + }
>
> Why don't we just check if f->index is within the [0, ARRAY_SIZE()-1] bounds
> and then just use it to index the array directly?
>
I will refine as :
static int mtk_fd_m2m_enum_fmt_out_mp(struct file *file, void *fh,
struct v4l2_fmtdesc *f)
{
if ((f->index >= 0) && (f->index < NUM_FORMATS)) {
f->pixelformat = in_img_fmts[f->index].pixelformat;
return 0;
}
return -EINVAL;
}
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int mtk_fd_m2m_try_fmt_out_mp(struct file *file,
> > + void *fh,
> > + struct v4l2_format *f)
>
> I think we could just shorten the function prefixes to "mtk_fd_".
>
Do you mean by replace mtk_fd_m2m_* with mtk_fd_ ?
> > +{
> > + struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > + const struct v4l2_pix_format_mplane *fmt;
> > +
> > + fmt = mtk_fd_find_fmt(pix_mp->pixelformat);
> > + if (!fmt) {
> > + /* Get default img fmt */
> > + fmt = &in_img_fmts[0];
> > + f->fmt.pix.pixelformat = fmt->pixelformat;
> > + }
> > +
> > + /* Use default */
> > + pix_mp->field = fmt->field;
> > + pix_mp->colorspace = fmt->colorspace;
> > + pix_mp->num_planes = fmt->num_planes;
> > + pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > + pix_mp->quantization = V4L2_QUANTIZATION_DEFAULT;
> > + pix_mp->xfer_func =
> > + V4L2_MAP_XFER_FUNC_DEFAULT(pix_mp->colorspace);
> > +
> > + /* Keep user setting as possible */
> > + pix_mp->width = clamp(pix_mp->width,
> > + MTK_FD_OUTPUT_MIN_WIDTH,
> > + MTK_FD_OUTPUT_MAX_WIDTH);
> > + pix_mp->height = clamp(pix_mp->height,
> > + MTK_FD_OUTPUT_MIN_HEIGHT,
> > + MTK_FD_OUTPUT_MAX_HEIGHT);
> > +
> > + pix_mp->plane_fmt[0].bytesperline = pix_mp->width * 2;
>
> Please add a comment explaining why this is always 2.
>
Done.
/* 2 bytes per pixel */
> > + pix_mp->plane_fmt[0].sizeimage =
> > + pix_mp->plane_fmt[0].bytesperline * pix_mp->height;
> > + memset(pix_mp->plane_fmt[0].reserved, 0,
> > + sizeof(pix_mp->plane_fmt[0].reserved));
>
> No need to zero this here. The core will handle it.
>
Ok, I will remove it.
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_g_fmt_out_mp(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_fd_ctx *ctx = fh_to_ctx(fh);
> > +
> > + f->fmt.pix_mp = ctx->src_fmt;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_s_fmt_out_mp(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_fd_ctx *ctx = fh_to_ctx(fh);
> > + struct vb2_queue *vq;
> > +
> > + /* Change not allowed if queue is streaming. */
> > + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > + if (vb2_is_streaming(vq) || vb2_is_busy(vq)) {
>
> vb2_is_busy() is a superset of vb2_is_streaming(), so it's enough to just
> check the former.
>
Ok, I will just check the former.
> > + dev_dbg(ctx->dev, "vb2_is_streaming or vb2_is_busy");
> > + return -EBUSY;
> > + }
> > +
> > + mtk_fd_m2m_try_fmt_out_mp(file, fh, f);
> > + ctx->src_fmt = f->fmt.pix_mp;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_enum_fmt_meta_cap(struct file *file, void *fh,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + if (f->index)
> > + return -EINVAL;
> > +
> > + strscpy(f->description, "Face detection result",
> > + sizeof(f->description));
> > + f->pixelformat = fw_param_fmts[0].dataformat;
> > + f->flags = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_g_fmt_meta_cap(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + f->fmt.meta.dataformat = fw_param_fmts[0].dataformat;
> > + f->fmt.meta.buffersize = fw_param_fmts[0].buffersize;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct vb2_ops mtk_fd_vb2_ops = {
> > + .queue_setup = mtk_fd_vb2_queue_setup,
> > + .buf_out_validate = mtk_fd_vb2_buf_out_validate,
> > + .buf_prepare = mtk_fd_vb2_buf_prepare,
> > + .buf_queue = mtk_fd_vb2_buf_queue,
> > + .start_streaming = mtk_fd_vb2_start_streaming,
> > + .stop_streaming = mtk_fd_vb2_stop_streaming,
> > + .wait_prepare = vb2_ops_wait_prepare,
> > + .wait_finish = vb2_ops_wait_finish,
> > + .buf_request_complete = mtk_fd_vb2_request_complete,
> > +};
> > +
> > +static const struct v4l2_ioctl_ops mtk_fd_v4l2_video_out_ioctl_ops = {
> > + .vidioc_querycap = mtk_fd_m2m_querycap,
> > + .vidioc_enum_fmt_vid_out_mplane = mtk_fd_m2m_enum_fmt_out_mp,
> > + .vidioc_g_fmt_vid_out_mplane = mtk_fd_m2m_g_fmt_out_mp,
> > + .vidioc_s_fmt_vid_out_mplane = mtk_fd_m2m_s_fmt_out_mp,
> > + .vidioc_try_fmt_vid_out_mplane = mtk_fd_m2m_try_fmt_out_mp,
> > + .vidioc_enum_fmt_meta_cap = mtk_fd_m2m_enum_fmt_meta_cap,
> > + .vidioc_g_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > + .vidioc_s_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > + .vidioc_try_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > + .vidioc_reqbufs = v4l2_m2m_ioctl_reqbufs,
> > + .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
> > + .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
> > + .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
> > + .vidioc_querybuf = v4l2_m2m_ioctl_querybuf,
> > + .vidioc_qbuf = v4l2_m2m_ioctl_qbuf,
> > + .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
> > + .vidioc_streamon = v4l2_m2m_ioctl_streamon,
> > + .vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
> > + .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> > + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> > +};
> > +
> > +static int
> > +mtk_fd_queue_init(void *priv, struct vb2_queue *src_vq,
> > + struct vb2_queue *dst_vq)
> > +{
> > + struct mtk_fd_ctx *ctx = priv;
> > + int ret;
> > +
> > + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > + src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> > + src_vq->supports_requests = true;
> > + src_vq->drv_priv = ctx;
> > + src_vq->ops = &mtk_fd_vb2_ops;
> > + src_vq->mem_ops = &vb2_dma_contig_memops;
> > + src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > + src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + src_vq->lock = &ctx->fd_dev->vfd_lock;
> > + src_vq->dev = ctx->fd_dev->v4l2_dev.dev;
> > +
> > + ret = vb2_queue_init(src_vq);
> > + if (ret)
> > + return ret;
> > +
> > + dst_vq->type = V4L2_BUF_TYPE_META_CAPTURE;
> > + dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> > + dst_vq->drv_priv = ctx;
> > + dst_vq->ops = &mtk_fd_vb2_ops;
> > + dst_vq->mem_ops = &vb2_dma_contig_memops;
> > + dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > + dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > + dst_vq->lock = &ctx->fd_dev->vfd_lock;
> > + dst_vq->dev = ctx->fd_dev->v4l2_dev.dev;
> > +
> > + return vb2_queue_init(dst_vq);
> > +}
> > +
> > +static int mtk_fd_dev_g_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mtk_fd_ctx *ctx = ctrl->priv;
> > + int i;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_MTK_FD_SCALE_IMG_WIDTH:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctrl->p_new.p_u16[i] =
> > + ctx->user_param.scale_img_width[i];
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctrl->p_new.p_u16[i] =
> > + ctx->user_param.scale_img_height[i];
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_POSE:
> > + ctrl->val = ctx->user_param.fd_pose;
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_SPEEDUP:
> > + ctrl->val = ctx->user_param.fd_speedup;
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_NUM:
> > + ctrl->val = ctx->user_param.scale_img_num;
> > + break;
> > + case V4L2_CID_MTK_FD_EXTRA_MODEL:
> > + ctrl->val = ctx->user_param.fd_extra_model;
> > + break;
> > + default:
> > + dev_dbg(ctx->dev, "%s: unexpected control: 0x%x:%d",
> > + __func__, ctrl->id, ctrl->val);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_dev_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mtk_fd_ctx *ctx = ctrl->priv;
> > + int i;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_MTK_FD_SCALE_IMG_WIDTH:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctx->user_param.scale_img_width[i] =
> > + ctrl->p_new.p_u16[i];
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT:
> > + for (i = 0; i < ctrl->elems; i++)
> > + ctx->user_param.scale_img_height[i] =
> > + ctrl->p_new.p_u16[i];
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_POSE:
> > + ctx->user_param.fd_pose = ctrl->val;
> > + break;
> > + case V4L2_CID_MTK_FD_DETECT_SPEEDUP:
> > + ctx->user_param.fd_speedup = ctrl->val;
> > + break;
> > + case V4L2_CID_MTK_FD_SCALE_IMG_NUM:
> > + ctx->user_param.scale_img_num = ctrl->val;
> > + break;
> > + case V4L2_CID_MTK_FD_EXTRA_MODEL:
> > + ctx->user_param.fd_extra_model = ctrl->val;
> > + break;
> > + default:
> > + dev_dbg(ctx->dev, "%s: unexpected control: 0x%x:%d",
> > + __func__, ctrl->id, ctrl->val);
> > + return -EINVAL;
>
> No need to handle this, as the framework will only pass the controls we
> registered earlier to this function.
>
Ok, I will remove this .
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops mtk_fd_dev_ctrl_ops = {
> > + .g_volatile_ctrl = mtk_fd_dev_g_ctrl,
>
> I don't see any volatile controls below. No need to implement this callback.
>
Ok, I will remove this .
> > + .s_ctrl = mtk_fd_dev_s_ctrl,
>
> I think you might not even need to implement this function (or just provide
> a dummy one that returns 0 if its required) if you just use the controls
> directly when preparing the job for the hardware. Check v4l2_ctrl_find().
>
I will remove the mtk_fd_dev_ctrl_ops, and use the following function to
get control values when preparing the job for HW,
static void mtk_fd_fill_user_param(struct user_param *user_param,
struct v4l2_ctrl_handler *hdl)
{
struct v4l2_ctrl *ctrl;
int i;
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_WIDTH);
if (ctrl)
for (i = 0; i < ctrl->elems; i++)
user_param->scale_img_width[i] = ctrl->p_new.p_u16[i];
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT);
if (ctrl)
for (i = 0; i < ctrl->elems; i++)
user_param->scale_img_height[i] = ctrl->p_new.p_u16[i];
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECT_POSE);
if (ctrl)
user_param->fd_pose = ctrl->val;
pr_info("pose:%d\n", user_param->fd_pose);
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECT_SPEEDUP);
if (ctrl)
user_param->fd_speedup = ctrl->val;
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_NUM);
if (ctrl)
user_param->scale_img_num = ctrl->val;
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_EXTRA_MODEL);
if (ctrl)
user_param->fd_extra_model = ctrl->val;
}
> > +};
> > +
> > +struct v4l2_ctrl_config mtk_fd_controls[] = {
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_SCALE_IMG_WIDTH,
> > + .name = "FD scale image widths",
> > + .type = V4L2_CTRL_TYPE_U16,
> > + .min = MTK_FD_OUTPUT_MIN_WIDTH,
> > + .max = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .step = 1,
> > + .def = MTK_FD_OUTPUT_MAX_WIDTH,
> > + .dims = { FD_SCALE_NUM },
>
> Something wrong with indentation here.
>
Done.
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT,
> > + .name = "FD scale image heights",
> > + .type = V4L2_CTRL_TYPE_U16,
> > + .min = MTK_FD_OUTPUT_MIN_HEIGHT,
> > + .max = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .step = 1,
> > + .def = MTK_FD_OUTPUT_MAX_HEIGHT,
> > + .dims = { FD_SCALE_NUM },
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_SCALE_IMG_NUM,
> > + .name = "FD scale size counts",
> > + .type = V4L2_CTRL_TYPE_INTEGER,
> > + .min = 0,
> > + .max = FACE_SIZE_NUM_MAX,
> > + .step = 1,
> > + .def = 0,
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_DETECT_POSE,
> > + .name = "FD detect face pose",
> > + .type = V4L2_CTRL_TYPE_INTEGER,
> > + .min = 0,
> > + .max = 3,
> > + .step = 1,
> > + .def = 0,
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_DETECT_SPEEDUP,
> > + .name = "FD detection speedup",
> > + .type = V4L2_CTRL_TYPE_INTEGER,
> > + .min = 0,
> > + .max = FD_MAX_SPEEDUP,
> > + .step = 1,
> > + .def = 0,
> > + },
> > + {
> > + .ops = &mtk_fd_dev_ctrl_ops,
> > + .id = V4L2_CID_MTK_FD_EXTRA_MODEL,
> > + .name = "FD use extra model",
> > + .type = V4L2_CTRL_TYPE_BOOLEAN,
> > + .min = 0,
> > + .max = 1,
> > + .step = 1,
> > + .def = 0,
> > + },
> > +};
> > +
> > +static int mtk_fd_ctrls_setup(struct mtk_fd_ctx *ctx)
> > +{
> > + struct v4l2_ctrl_handler *hdl = &ctx->hdl;
> > + struct v4l2_ctrl *ctl;
> > + int i;
> > +
> > + v4l2_ctrl_handler_init(hdl, V4L2_CID_MTK_FD_MAX);
> > + if (hdl->error)
> > + return hdl->error;
> > +
> > + for (i = 0; i < ARRAY_SIZE(mtk_fd_controls); i++) {
> > + ctl = v4l2_ctrl_new_custom(hdl, &mtk_fd_controls[i], ctx);
> > + if (hdl->error) {
> > + v4l2_ctrl_handler_free(hdl);
> > + dev_err(ctx->dev, "Failed to register controls:%d", i);
> > + return hdl->error;
> > + }
> > + }
> > +
> > + ctx->fh.ctrl_handler = &ctx->hdl;
> > + v4l2_ctrl_handler_setup(hdl);
> > +
> > + return 0;
> > +}
> > +
> > +static unsigned int get_fd_img_fmt(unsigned int fourcc)
> > +{
> > + switch (fourcc) {
> > + case V4L2_PIX_FMT_VYUY:
> > + return FMT_VYUY;
> > + case V4L2_PIX_FMT_YUYV:
> > + return FMT_YUYV;
> > + case V4L2_PIX_FMT_YVYU:
> > + return FMT_YVYU;
> > + case V4L2_PIX_FMT_UYVY:
> > + return FMT_UYVY;
> > + default:
> > + return FMT_UNKNOWN;
> > + }
> > +}
> > +
> > +static void init_ctx_fmt(struct mtk_fd_ctx *ctx)
> > +{
> > + const struct v4l2_pix_format_mplane *fmt;
> > +
> > + /* Initialize M2M source fmt */
> > + fmt = &in_img_fmts[0];
> > + mtk_fd_fill_pixfmt_mp(&ctx->src_fmt, fmt);
> > +
> > + /* Initialize M2M destination fmt */
> > + ctx->dst_fmt.buffersize = fw_param_fmts[0].buffersize;
> > + ctx->dst_fmt.dataformat = fw_param_fmts[0].dataformat;
>
> Why not just make dst_fmt a pointer and assign &fw_params_fmt[0] there?
>
fw_params_fmt[] will be removed and assign the const value here.
ctx->dst_fmt.buffersize = sizeof(struct fd_user_output);
ctx->dst_fmt.dataformat = V4L2_META_FMT_MTFD_RESULT;
> > +}
> > +
> > +/*
> > + * V4L2 file operations.
> > + */
> > +static int mtk_vfd_open(struct file *filp)
> > +{
> > + struct mtk_fd_dev *fd_dev = video_drvdata(filp);
> > + struct video_device *vdev = video_devdata(filp);
> > + struct mtk_fd_ctx *ctx;
> > + int ret;
> > +
> > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx)
> > + return -ENOMEM;
> > +
> > + ctx->fd_dev = fd_dev;
> > + ctx->dev = &fd_dev->pdev->dev;
> > +
> > + v4l2_fh_init(&ctx->fh, vdev);
> > + filp->private_data = &ctx->fh;
> > +
> > + init_ctx_fmt(ctx);
> > +
> > + ret = mtk_fd_ctrls_setup(ctx);
> > + if (ret) {
> > + dev_err(ctx->dev, "Failed to set up controls:%d\n", ret);
> > + goto err_fh_ctrl;
> > + }
> > +
> > + ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(fd_dev->m2m_dev, ctx,
> > + &mtk_fd_queue_init);
> > + if (IS_ERR(ctx->fh.m2m_ctx)) {
> > + ret = PTR_ERR(ctx->fh.m2m_ctx);
> > + goto err_init_ctx;
> > + }
> > +
> > + v4l2_fh_add(&ctx->fh);
> > +
> > + return 0;
> > +
> > +err_init_ctx:
> > + v4l2_ctrl_handler_free(&ctx->hdl);
> > +err_fh_ctrl:
> > + v4l2_fh_exit(&ctx->fh);
> > + kfree(ctx);
> > +
> > + return ret;
> > +}
> > +
> > +static int mtk_vfd_release(struct file *filp)
> > +{
> > + struct mtk_fd_ctx *ctx = container_of(filp->private_data,
> > + struct mtk_fd_ctx, fh);
> > +
> > + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> > +
> > + v4l2_ctrl_handler_free(&ctx->hdl);
> > + v4l2_fh_del(&ctx->fh);
> > + v4l2_fh_exit(&ctx->fh);
> > +
> > + kfree(ctx);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_file_operations fd_video_fops = {
> > + .owner = THIS_MODULE,
> > + .open = mtk_vfd_open,
> > + .release = mtk_vfd_release,
> > + .poll = v4l2_m2m_fop_poll,
> > + .unlocked_ioctl = video_ioctl2,
> > + .mmap = v4l2_m2m_fop_mmap,
>
> We also need the compat_ioctl here for 32-bit userspace on 64-bit kernels.
>
Done.
> > +};
> > +
> > +static void mtk_fd_device_run(void *priv)
> > +{
> > + struct mtk_fd_ctx *ctx = priv;
> > + struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > + struct fd_hw_param fd_param;
> > + void *fd_result_vaddr;
> > +
> > + src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > +
> > + memset(&fd_param, 0, sizeof(fd_param));
> > +
> > + fd_param.src_img.dma_addr =
> > + vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
> > + fd_param.user_result.dma_addr =
> > + vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
> > + fd_result_vaddr = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> > +
> > + ctx->user_param.src_img_fmt = get_fd_img_fmt(ctx->src_fmt.pixelformat);
> > + memcpy(&fd_param.user_param, &ctx->user_param, sizeof(ctx->user_param));
> > +
> > + /* Complete request controls if any */
> > + v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, &ctx->hdl);
> > +
> > + mtk_fd_hw_job_exec(&ctx->fd_dev->fd_hw, &fd_param, fd_result_vaddr);
> > +}
> > +
> > +static struct v4l2_m2m_ops fd_m2m_ops = {
> > + .device_run = mtk_fd_device_run,
> > +};
> > +
> > +static int mtk_fd_request_validate(struct media_request *req)
> > +{
> > + unsigned int count;
> > +
> > + count = vb2_request_buffer_cnt(req);
> > + if (!count)
> > + return -ENOENT;
>
> Why -ENOENT?
>
Reference the return code in vb2_request_validate()
I consider refining as following:
static int mtk_fd_request_validate(struct media_request *req)
{
if (vb2_request_buffer_cnt(req) > 1)
return -EINVAL;
return vb2_request_validate(req);
}
or maybe I don't need to worry the request count greater than 1,
just remove this function and set vb2_request_validate as .req_validate
directly?
> > + else if (count > 1)
> > + return -EINVAL;
> > +
> > + return vb2_request_validate(req);
> > +}
> > +
> > +static const struct media_device_ops fd_m2m_media_ops = {
> > + .req_validate = mtk_fd_request_validate,
> > + .req_queue = v4l2_m2m_request_queue,
> > +};
> > +
> > +static int mtk_fd_video_device_register(struct mtk_fd_dev *fd_dev)
> > +{
> > + struct video_device *vfd = &fd_dev->vfd;
> > + struct v4l2_m2m_dev *m2m_dev = fd_dev->m2m_dev;
> > + struct device *dev = &fd_dev->pdev->dev;
> > + int function, ret;
> > +
> > + vfd->fops = &fd_video_fops;
> > + vfd->release = video_device_release;
> > + vfd->lock = &fd_dev->vfd_lock;
> > + vfd->v4l2_dev = &fd_dev->v4l2_dev;
> > + vfd->vfl_dir = VFL_DIR_M2M;
> > + vfd->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_OUTPUT_MPLANE |
> > + V4L2_CAP_META_CAPTURE;
> > + vfd->ioctl_ops = &mtk_fd_v4l2_video_out_ioctl_ops;
> > +
> > + strscpy(vfd->name, "MTK-FD-V4L2", sizeof(vfd->name));
>
> Please use dev_driver_string().
>
Done.
> > + video_set_drvdata(vfd, fd_dev);
> > +
> > + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> > + if (ret) {
> > + dev_err(dev, "Failed to register video device\n");
> > + goto err_free_dev;
> > + }
> > +
> > + function = MEDIA_ENT_F_PROC_VIDEO_DECODER;
>
> MEDIA_ENT_F_PROC_VIDEO_STATISTICS fits here much more.
>
> Also nit: Any reason to have this assigned to this intermediate variable
> rather than just directly passed to the function?
>
Too long for 80 columns, I will remove the intermediate variable and
refine as following:
ret = v4l2_m2m_register_media_controller(m2m_dev, vfd,
MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
> > + ret = v4l2_m2m_register_media_controller(m2m_dev, vfd, function);
> > + if (ret) {
> > + dev_err(dev, "Failed to init mem2mem media controller\n");
> > + goto err_unreg_video;
> > + }
> > + return 0;
> > +
> > +err_unreg_video:
> > + video_unregister_device(vfd);
> > +err_free_dev:
> > + video_device_release(vfd);
> > + return ret;
> > +}
> > +
> > +static int mtk_fd_dev_v4l2_init(struct mtk_fd_dev *fd_dev)
> > +{
> > + struct media_device *mdev = &fd_dev->mdev;
> > + struct device *dev = &fd_dev->pdev->dev;
> > + int ret;
> > +
> > + ret = v4l2_device_register(dev, &fd_dev->v4l2_dev);
> > + if (ret) {
> > + dev_err(dev, "Failed to register v4l2 device\n");
> > + return ret;
> > + }
> > +
> > + fd_dev->m2m_dev = v4l2_m2m_init(&fd_m2m_ops);
> > + if (IS_ERR(fd_dev->m2m_dev)) {
> > + dev_err(dev, "Failed to init mem2mem device\n");
> > + ret = PTR_ERR(fd_dev->m2m_dev);
> > + goto fail_m2m_dev;
>
> Please name the labels after the next clean-up step to be done, not the
> failure.
>
Ok, I will refine as following:
err_unreg_vdev:
v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
video_unregister_device(&fd_dev->vfd);
video_device_release(&fd_dev->vfd);
err_cleanup_mdev:
media_device_cleanup(mdev);
v4l2_m2m_release(fd_dev->m2m_dev);
err_unreg_v4l2_dev:
v4l2_device_unregister(&fd_dev->v4l2_dev);
> > + }
> > +
> > + mdev->dev = dev;
> > + strscpy(mdev->model, "MTK-FD-V4L2", sizeof(mdev->model));
>
> Could we just use dev_driver_string() here instead?
>
Done.
> > + snprintf(mdev->bus_info, sizeof(mdev->bus_info),
> > + "platform:%s", dev_name(dev));
> > + media_device_init(mdev);
> > + mdev->ops = &fd_m2m_media_ops;
> > + fd_dev->v4l2_dev.mdev = mdev;
> > +
> > + ret = mtk_fd_video_device_register(fd_dev);
> > + if (ret) {
> > + dev_err(dev, "Failed to register video device\n");
> > + goto err_vdev;
> > + }
> > +
> > + ret = media_device_register(mdev);
> > + if (ret) {
> > + dev_err(dev, "Failed to register mem2mem media device\n");
> > + goto fail_mdev;
> > + }
> > +
> > + return 0;
> > +
> > +fail_mdev:
> > + v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
> > + video_unregister_device(&fd_dev->vfd);
> > + video_device_release(&fd_dev->vfd);
> > +err_vdev:
> > + media_device_cleanup(mdev);
> > + v4l2_m2m_release(fd_dev->m2m_dev);
> > +fail_m2m_dev:
> > + v4l2_device_unregister(&fd_dev->v4l2_dev);
> > + return ret;
> > +}
> > +
> > +static void mtk_fd_dev_v4l2_release(struct mtk_fd_dev *fd_dev)
> > +{
> > + v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
> > + video_unregister_device(&fd_dev->vfd);
> > + video_device_release(&fd_dev->vfd);
> > + media_device_cleanup(&fd_dev->mdev);
> > + v4l2_m2m_release(fd_dev->m2m_dev);
> > + v4l2_device_unregister(&fd_dev->v4l2_dev);
> > +}
> > +
> > +static irqreturn_t mtk_fd_irq(int irq, void *data)
> > +{
> > + struct mtk_fd_hw *fd_hw = (struct mtk_fd_hw *)data;
> > +
> > + fd_hw->fd_irq_result = readl(fd_hw->fd_base + FD_INT);
>
> We should be able to just handle the job completion directly from here.
>
Ok, I will try to handle the job completion from here.
> > + wake_up_interruptible(&fd_hw->wq);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_fd_hw_get_scp_mem(struct mtk_fd_hw *fd_hw,
> > + struct fd_buffer *scp_mem)
> > +{
> > + struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > + struct device *dev = &fd_dev->pdev->dev;
> > + dma_addr_t addr;
> > + u32 size;
> > +
> > + scp_mem->scp_addr = scp_get_reserve_mem_phys(SCP_FD_MEM_ID);
> > + size = scp_get_reserve_mem_size(SCP_FD_MEM_ID);
> > + if (!scp_mem->scp_addr || !size)
> > + return -EPROBE_DEFER;
>
> Why -EPROBE_DEFER? Should we have some way to distinguish between the SCP
> driver not being initialized yet and some other error?
>
Ok, I will fix it to -ENOMEM.
Not being initialized or some other error can be found during device
probe.
(will also load scp stuff there)
> > +
> > + /* get dma addr address */
> > + addr = dma_map_page_attrs(dev, phys_to_page(scp_mem->scp_addr), 0,
> > + size, DMA_BIDIRECTIONAL,
>
> Should this be really bidirectional?
>
I will fix it to DMA_TO_DEVICE
> > + DMA_ATTR_SKIP_CPU_SYNC);
> > + if (dma_mapping_error(dev, addr)) {
> > + scp_mem->scp_addr = 0;
> > + dev_err(dev, "Failed to map scp addr\n");
> > + return -ENOMEM;
> > + }
> > + scp_mem->dma_addr = addr;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_probe(struct platform_device *pdev)
> > +{
> > + struct mtk_fd_dev *fd_dev;
>
> nit: Perhaps just fd, to have shorter code?
>
Ok, I will refine as
struct mtk_fd_dev *fd;
> > + struct device *dev = &pdev->dev;
> > + struct mtk_fd_hw *fd_hw;
> > + struct resource *res;
> > + int irq;
> > + int ret;
> > +
> > + fd_dev = devm_kzalloc(&pdev->dev, sizeof(*fd_dev), GFP_KERNEL);
> > + if (!fd_dev)
> > + return -ENOMEM;
> > +
> > + dev_set_drvdata(dev, fd_dev);
> > + fd_hw = &fd_dev->fd_hw;
> > + fd_dev->pdev = pdev;
>
> Is pdev useful for anything? Perhaps you want to store &pdev->dev instead?
>
Yes, I will store that instead.
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(dev, "no IRQ:%d resource info\n", irq);
> > + return irq;
> > + }
> > + ret = devm_request_irq(dev, irq, mtk_fd_irq, IRQF_SHARED,
> > + dev_driver_string(dev),
> > + fd_hw);
>
> Why not just fd_dev?
>
Ok, I will use fd_dev here.
> > + if (ret) {
> > + dev_err(dev, "req_irq fail:%d\n", irq);
> > + return ret;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + fd_hw->fd_base = devm_ioremap_resource(dev, res);
> > + if (!fd_hw->fd_base) {
> > + dev_err(dev, "unable to get fd reg base\n");
> > + return PTR_ERR(fd_hw->fd_base);
> > + }
> > +
> > + fd_hw->fd_clk = devm_clk_get(dev, "fd");
> > + if (IS_ERR(fd_hw->fd_clk)) {
> > + dev_err(dev, "cannot get fd_clk_img_fd clock\n");
> > + return PTR_ERR(fd_hw->fd_clk);
> > + }
> > +
> > + ret = mtk_fd_hw_get_scp_mem(fd_hw, &fd_hw->scp_mem);
> > + if (ret) {
> > + dev_err(dev, "scp memory init failed: %d\n", ret);
>
> nit: Could we make the error messages a bit more consistent? For example
> "Failed to get IRQ resource (%d)", "Failed to request IRQ (%d)", "Failed to
> ... (%d)", etc.
>
Ok, I will fix it.
> > + return ret;
> > + }
> > +
> > + atomic_set(&fd_hw->fd_user_cnt, 0);
> > + init_waitqueue_head(&fd_hw->wq);
> > + mutex_init(&fd_dev->vfd_lock);
> > + mutex_init(&fd_hw->fd_hw_lock);
> > + pm_runtime_enable(dev);
> > +
> > + ret = mtk_fd_dev_v4l2_init(fd_dev);
> > + if (ret) {
> > + mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> > + mutex_destroy(&fd_dev->vfd_lock);
> > + pm_runtime_disable(&pdev->dev);
>
> Please move the clean-up to an error path on the bottom of the function.
>
Done.
> > + dev_err(dev, "v4l2 init failed: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_remove(struct platform_device *pdev)
> > +{
> > + struct mtk_fd_dev *fd_dev = dev_get_drvdata(&pdev->dev);
> > +
> > + mtk_fd_dev_v4l2_release(fd_dev);
> > + mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> > + mutex_destroy(&fd_dev->vfd_lock);
> > + pm_runtime_disable(&pdev->dev);
>
> The order of calls in remove should normally be the opposite to probe.
>
Ok, I will refine as following
mtk_fd_dev_v4l2_release(fd_dev);
pm_runtime_disable(&pdev->dev);
mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
mutex_destroy(&fd_dev->vfd_lock);
rproc_put(fd_dev->fd_hw.rproc_handle);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_suspend(struct device *dev)
> > +{
> > + struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
> > +
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
> > + /* suspend FD HW */
> > + writel(0x0, fd_dev->fd_hw.fd_base + FD_HW_ENABLE);
> > + writel(0x0, fd_dev->fd_hw.fd_base + FD_INT_EN);
> > + clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> > + return 0;
> > +}
> > +
> > +static int mtk_fd_resume(struct device *dev)
> > +{
> > + struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
> > + ret = clk_prepare_enable(fd_dev->fd_hw.fd_clk);
> > + if (ret < 0) {
> > + dev_dbg(dev, "open fd clk failed\n");
> > + clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> > + }
> > +
> > + /* resume FD HW */
> > + writel(ENABLE_FD, fd_dev->fd_hw.fd_base + FD_HW_ENABLE);
> > + if (fd_dev->fd_hw.state == FD_ENQ)
> > + writel(0x1, fd_dev->fd_hw.fd_base + FD_INT_EN);
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_fd_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mtk_fd_suspend, mtk_fd_resume)
> > + SET_RUNTIME_PM_OPS(mtk_fd_suspend, mtk_fd_resume, NULL)
>
> We need separate runtime and system PM ops. Their behavior is expected to be
> different.
>
> For runtime PM ops, you the functions should just unconditionally power on
> or power off the device.
>
> For system PM ops, suspend needs to make sure that no further job is queued
> to the hardware and that any job being already processed by the hardware is
> completed. Resume needs to resume the processing if there are any further
> jobs to be queued to the hardware.
>
Ok, for runtime suspend/resume I will add the following functions:
static int mtk_fd_runtime_suspend(struct device *dev)
{
struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
dev_dbg(dev, "%s:disable clock\n", __func__);
clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
return 0;
}
static int mtk_fd_runtime_resume(struct device *dev)
{
struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
int ret;
dev_dbg(dev, "%s:enable clock\n", __func__);
ret = clk_prepare_enable(fd_dev->fd_hw.fd_clk);
if (ret < 0) {
dev_err(dev, "Failed to open fd clk:%d\n", ret);
return ret;
}
return 0;
}
Best regards,
Jerry
> > +};
> > +
> > +static const struct of_device_id mtk_fd_of_ids[] = {
> > + { .compatible = "mediatek,mt8183-fd", },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_fd_of_ids);
> > +
> > +static struct platform_driver mtk_fd_driver = {
> > + .probe = mtk_fd_probe,
> > + .remove = mtk_fd_remove,
> > + .driver = {
> > + .name = "mtk-fd-4.0",
> > + .of_match_table = of_match_ptr(mtk_fd_of_ids),
> > + .pm = &mtk_fd_pm_ops,
> > + }
> > +};
> > +module_platform_driver(mtk_fd_driver);
> > +
> > +MODULE_DESCRIPTION("Mediatek FD driver");
> > +MODULE_LICENSE("GPL");
>
> GPL v2
>
> Best regards,
> Tomasz
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v3 2/3] net: ethernet: mediatek: Re-add support SGMII
From: René van Dorst @ 2019-08-25 11:41 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Nelson Chang, Frank Wunderlich, netdev, Sean Wang, linux-mips,
linux-mediatek, John Crispin, Matthias Brugger, Stefan Roese,
David S . Miller, linux-arm-kernel
In-Reply-To: <20190824133225.GE13294@shell.armlinux.org.uk>
Hi Russell,
Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>:
> Hi René,
>
> On Sat, Aug 24, 2019 at 01:11:17PM +0000, René van Dorst wrote:
>> Hi Russell,
>>
>> Mediatek calls it Turbo RGMII. It is a overclock version of RGMII mode.
>> It is used between first GMAC and port 6 of the mt7530 switch. Can be used
>> with
>> an internal and an external mt7530 switch.
>>
>> TRGMII speed are:
>> * mt7621: 1200Mbit
>> * mt7623: 2000Mbit and 2600Mbit.
>>
>> I think that TRGMII is only used in a fixed-link situation in combination
>> with a
>> mt7530 switch and running and maximum speed/full duplex. So reporting
>> 1000baseT_Full seems to me the right option.
>
> I think we can ignore this one for the purposes of merging this patch
> set, since this seems to be specific to this setup. Neither 1000BaseT
> nor 1000BaseX fit very well, but we have to choose something.
>
>> PHY_INTERFACE_MODE_GMII:
>> 10baseT_Half
>> 10baseT_Full
>> 100baseT_Half
>> 100baseT_Full
>> 1000baseT_Half
>> 1000baseT_Full
>
> I think GMII can be connected to a PHY that can convert to 1000BaseX, so
> should probably include that here too.
>
Thanks for reviewing.
I shall add that too.
I send v4 today.
Greats,
René
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [GIT PULL] i.MX clock changes for 5.4
From: Shawn Guo @ 2019-08-25 11:55 UTC (permalink / raw)
To: Stephen Boyd
Cc: Stefan Agner, linux-imx, kernel, Fabio Estevam, linux-clk,
linux-arm-kernel
Hi Stephen,
This is the i.MX clock changes I collected for 5.4. Please help pull,
and keep commit 6ad7cb7122ce ("clk: imx8: Add DSP related clocks")
stable, as I pulled it into my DT branch as dependency. Thanks!
Shawn
The following changes since commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b:
Linus 5.3-rc1 (2019-07-21 14:05:38 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git tags/clk-imx-5.4
for you to fetch changes up to 760e548e7f885d89bf2dfab4838df9379edd19fc:
clk: imx: imx8mn: fix audio pll setting (2019-08-24 21:04:27 +0200)
----------------------------------------------------------------
i.MX clock changes for 5.4:
- Add clock driver for i.MX8MN SoC.
- Switch i.MX8MM clock driver to platform driver.
- Add API for clk unregister when driver probe fail.
- Add Hifi4 DSP related clocks for i.MX8QXP SoC.
- Fix Audio PLL setting and parent clock for USB.
- Misc i.MX8 clock driver improvements and corrections.
----------------------------------------------------------------
Abel Vesa (3):
clk: imx: Remove unused clk based API
clk: imx8mm: Switch to platform driver
clk: imx8mq: Mark AHB clock as critical
Anson Huang (14):
dt-bindings: imx: Add clock binding doc for i.MX8MN
clk: imx8mm: Make 1416X/1443X PLL macro definitions common for usage
clk: imx: Add API for clk unregister when driver probe fail
clk: imx: Add support for i.MX8MN clock driver
clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT
clk: imx8mm: Fix typo of pwm3 clock's mux option #4
clk: imx8mm: GPT1 clock mux option #5 should be sys_pll1_80m
clk: imx7ulp: Make sure earlycon's clock is enabled
clk: imx: Remove unused function statement
clk: imx8mn: Keep uart clocks on for early console
clk: imx8mm: Unregister clks when of_clk_add_provider failed
clk: imx8mq: Unregister clks when of_clk_add_provider failed
clk: imx8mn: Add missing rate_count assignment for each PLL structure
clk: imx8mn: Add necessary frequency support for ARM PLL table
Daniel Baluta (1):
clk: imx8: Add DSP related clocks
Fancy Fang (1):
clk: imx8mm: rename 'share_count_dcss' to 'share_count_disp'
Leonard Crestez (4):
clk: imx8mq: Fix sys3 pll references
clk: imx8mm: Fix incorrect parents
clk: imx8mn: Fix incorrect parents
clk: imx8mn: Add GIC clock
Li Jun (2):
clk: imx8mm: correct the usb1_ctrl parent to be usb_bus
clk: imx8mq: set correct parent for usb ctrl clocks
Peng Fan (3):
clk: imx: imx8mm: fix audio pll setting
clk: imx8mn: fix int pll clk gate
clk: imx: imx8mn: fix audio pll setting
.../devicetree/bindings/clock/imx8mn-clock.yaml | 112 ++++
drivers/clk/imx/Kconfig | 6 +
drivers/clk/imx/Makefile | 1 +
drivers/clk/imx/clk-imx7ulp.c | 31 +
drivers/clk/imx/clk-imx8mm.c | 109 ++--
drivers/clk/imx/clk-imx8mn.c | 660 +++++++++++++++++++++
drivers/clk/imx/clk-imx8mq.c | 131 ++--
drivers/clk/imx/clk-imx8qxp-lpcg.c | 5 +
drivers/clk/imx/clk.c | 8 +
drivers/clk/imx/clk.h | 43 +-
include/dt-bindings/clock/imx8-clock.h | 6 +-
include/dt-bindings/clock/imx8mn-clock.h | 216 +++++++
12 files changed, 1188 insertions(+), 140 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/imx8mn-clock.yaml
create mode 100644 drivers/clk/imx/clk-imx8mn.c
create mode 100644 include/dt-bindings/clock/imx8mn-clock.h
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v2 0/4] wm8904: adapt driver for use with audio-graph-card
From: Michał Mirosław @ 2019-08-25 12:17 UTC (permalink / raw)
To: alsa-devel, patches
Cc: Kate Stewart, Maxime Jourdan, Alexandre Belloni,
Kuninori Morimoto, Kirill Marinushkin, Liam Girdwood,
Paul Cercueil, Srinivas Kandagatla, Jerome Brunet, Anders Roxell,
Takashi Iwai, Ludovic Desroches, linux-arm-kernel,
Codrin Ciubotariu, Charles Keepax, Piotr Stankiewicz,
Annaliese McDermond, Richard Fitzgerald, Mark Brown,
Nariman Poushin, Thomas Gleixner, Jaroslav Kysela, zhong jiang,
Allison Randal, Greg Kroah-Hartman, Randy Dunlap, Nikesh Oswal,
linux-kernel, Enrico Weigelt
This series allows to use WM8904 codec as audio-graph-card component.
It starts with rework of FLL handling in the codec's driver, and as an
example includes (untested) rework for codec with similar FLL: WM8994.
Series based on tiwai/sound/for-next tree. You can also pull from:
https://rere.qmqm.pl/git/linux
branch:
wm8904
(branch includes two fixes already sent to alsa-devel, but not merged yet).
Michał Mirosław (4):
ASoC: wm_fll: extract common code for Wolfson FLLs
ASoC: wm8904: use common FLL code
ASoC: wm8904: automatically choose clock source
[RFT] ASoC: wm8994: use common FLL code
sound/soc/atmel/atmel_wm8904.c | 11 +-
sound/soc/codecs/Kconfig | 9 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/wm8904.c | 516 +++++++++++---------------------
sound/soc/codecs/wm8904.h | 5 -
sound/soc/codecs/wm8994.c | 281 +++++-------------
sound/soc/codecs/wm8994.h | 4 +-
sound/soc/codecs/wm_fll.c | 518 +++++++++++++++++++++++++++++++++
sound/soc/codecs/wm_fll.h | 60 ++++
9 files changed, 849 insertions(+), 557 deletions(-)
create mode 100644 sound/soc/codecs/wm_fll.c
create mode 100644 sound/soc/codecs/wm_fll.h
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v2 4/4] [RFT] ASoC: wm8994: use common FLL code
From: Michał Mirosław @ 2019-08-25 12:17 UTC (permalink / raw)
To: alsa-devel, patches
Cc: Maxime Jourdan, Kate Stewart, Alexandre Belloni,
Kuninori Morimoto, Kirill Marinushkin, Liam Girdwood,
Paul Cercueil, Srinivas Kandagatla, Jerome Brunet, Anders Roxell,
Ludovic Desroches, linux-arm-kernel, Codrin Ciubotariu,
Charles Keepax, Piotr Stankiewicz, Nariman Poushin,
Richard Fitzgerald, Mark Brown, Annaliese McDermond,
Thomas Gleixner, Jaroslav Kysela, zhong jiang, Allison Randal,
Greg Kroah-Hartman, Randy Dunlap, Nikesh Oswal, Takashi Iwai,
linux-kernel, Enrico Weigelt
In-Reply-To: <cover.1566734630.git.mirq-linux@rere.qmqm.pl>
Rework FLL handling to use common code.
This uses polling for now to wait for FLL lock.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
sound/soc/codecs/Kconfig | 2 +
sound/soc/codecs/wm8994.c | 281 +++++++++++---------------------------
sound/soc/codecs/wm8994.h | 4 +-
3 files changed, 84 insertions(+), 203 deletions(-)
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 1a680023af7d..1ff6290ce18d 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -1382,6 +1382,8 @@ config SND_SOC_WM8993
config SND_SOC_WM8994
tristate
+ select SND_SOC_WM_FLL
+ select SND_SOC_WM_FLL_EFS
config SND_SOC_WM8995
tristate
diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
index c3d06e8bc54f..d0dbc352303b 100644
--- a/sound/soc/codecs/wm8994.c
+++ b/sound/soc/codecs/wm8994.c
@@ -2030,101 +2030,57 @@ static const struct snd_soc_dapm_route wm8958_intercon[] = {
{ "AIF3ADC Mux", "Mono PCM", "Mono PCM Out Mux" },
};
-/* The size in bits of the FLL divide multiplied by 10
- * to allow rounding later */
-#define FIXED_FLL_SIZE ((1 << 16) * 10)
-
-struct fll_div {
- u16 outdiv;
- u16 n;
- u16 k;
- u16 lambda;
- u16 clk_ref_div;
- u16 fll_fratio;
+static const struct wm_fll_desc wm8994_fll_desc[2] = {
+ /* FLL1 */
+ {
+ .ctl_offset = WM8994_FLL1_CONTROL_1,
+ .int_offset = WM8994_INTERRUPT_RAW_STATUS_2,
+ .int_mask = WM8994_IM_FLL1_LOCK_EINT_MASK,
+ .nco_reg0 = WM8994_FLL1_CONTROL_5,
+ .frc_nco_shift = 6,
+ .nco_reg1 = WM8994_FLL1_CONTROL_5,
+ .frc_nco_val_shift = 7,
+ .clk_ref_map = { FLL_REF_MCLK, FLL_REF_MCLK2, FLL_REF_FSCLK, FLL_REF_BCLK },
+ },
+ /* FLL2 */
+ {
+ .ctl_offset = WM8994_FLL2_CONTROL_1,
+ .int_offset = WM8994_INTERRUPT_RAW_STATUS_2,
+ .int_mask = WM8994_IM_FLL2_LOCK_EINT_MASK,
+ .nco_reg0 = WM8994_FLL2_CONTROL_5,
+ .frc_nco_shift = 6,
+ .nco_reg1 = WM8994_FLL2_CONTROL_5,
+ .frc_nco_val_shift = 7,
+ .clk_ref_map = { FLL_REF_MCLK, FLL_REF_MCLK2, FLL_REF_FSCLK, FLL_REF_BCLK },
+ },
};
-static int wm8994_get_fll_config(struct wm8994 *control, struct fll_div *fll,
- int freq_in, int freq_out)
-{
- u64 Kpart;
- unsigned int K, Ndiv, Nmod, gcd_fll;
-
- pr_debug("FLL input=%dHz, output=%dHz\n", freq_in, freq_out);
-
- /* Scale the input frequency down to <= 13.5MHz */
- fll->clk_ref_div = 0;
- while (freq_in > 13500000) {
- fll->clk_ref_div++;
- freq_in /= 2;
-
- if (fll->clk_ref_div > 3)
- return -EINVAL;
- }
- pr_debug("CLK_REF_DIV=%d, Fref=%dHz\n", fll->clk_ref_div, freq_in);
-
- /* Scale the output to give 90MHz<=Fvco<=100MHz */
- fll->outdiv = 3;
- while (freq_out * (fll->outdiv + 1) < 90000000) {
- fll->outdiv++;
- if (fll->outdiv > 63)
- return -EINVAL;
- }
- freq_out *= fll->outdiv + 1;
- pr_debug("OUTDIV=%d, Fvco=%dHz\n", fll->outdiv, freq_out);
-
- if (freq_in > 1000000) {
- fll->fll_fratio = 0;
- } else if (freq_in > 256000) {
- fll->fll_fratio = 1;
- freq_in *= 2;
- } else if (freq_in > 128000) {
- fll->fll_fratio = 2;
- freq_in *= 4;
- } else if (freq_in > 64000) {
- fll->fll_fratio = 3;
- freq_in *= 8;
- } else {
- fll->fll_fratio = 4;
- freq_in *= 16;
- }
- pr_debug("FLL_FRATIO=%d, Fref=%dHz\n", fll->fll_fratio, freq_in);
-
- /* Now, calculate N.K */
- Ndiv = freq_out / freq_in;
-
- fll->n = Ndiv;
- Nmod = freq_out % freq_in;
- pr_debug("Nmod=%d\n", Nmod);
-
- switch (control->type) {
- case WM8994:
- /* Calculate fractional part - scale up so we can round. */
- Kpart = FIXED_FLL_SIZE * (long long)Nmod;
-
- do_div(Kpart, freq_in);
-
- K = Kpart & 0xFFFFFFFF;
-
- if ((K % 10) >= 5)
- K += 5;
-
- /* Move down to proper range now rounding is done */
- fll->k = K / 10;
- fll->lambda = 0;
-
- pr_debug("N=%x K=%x\n", fll->n, fll->k);
- break;
-
- default:
- gcd_fll = gcd(freq_out, freq_in);
-
- fll->k = (freq_out - (freq_in * fll->n)) / gcd_fll;
- fll->lambda = freq_in / gcd_fll;
-
- }
-
- return 0;
-}
+static const struct wm_fll_desc wm8958_fll_desc[2] = {
+ /* FLL1 */
+ {
+ .ctl_offset = WM8994_FLL1_CONTROL_1,
+ .int_offset = WM8994_INTERRUPT_RAW_STATUS_2,
+ .int_mask = WM8994_IM_FLL1_LOCK_EINT_MASK,
+ .nco_reg0 = WM8994_FLL1_CONTROL_5,
+ .frc_nco_shift = 6,
+ .nco_reg1 = WM8994_FLL1_CONTROL_5,
+ .frc_nco_val_shift = 7,
+ .efs_offset = WM8958_FLL1_EFS_1,
+ .clk_ref_map = { FLL_REF_MCLK, FLL_REF_MCLK2, FLL_REF_FSCLK, FLL_REF_BCLK },
+ },
+ /* FLL2 */
+ {
+ .ctl_offset = WM8994_FLL2_CONTROL_1,
+ .int_offset = WM8994_INTERRUPT_RAW_STATUS_2,
+ .int_mask = WM8994_IM_FLL2_LOCK_EINT_MASK,
+ .nco_reg0 = WM8994_FLL2_CONTROL_5,
+ .frc_nco_shift = 6,
+ .nco_reg1 = WM8994_FLL2_CONTROL_5,
+ .frc_nco_val_shift = 7,
+ .efs_offset = WM8958_FLL1_EFS_1,
+ .clk_ref_map = { FLL_REF_MCLK, FLL_REF_MCLK2, FLL_REF_FSCLK, FLL_REF_BCLK },
+ },
+};
static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
unsigned int freq_in, unsigned int freq_out)
@@ -2132,9 +2088,7 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
struct wm8994_priv *wm8994 = snd_soc_component_get_drvdata(component);
struct wm8994 *control = wm8994->wm8994;
int reg_offset, ret;
- struct fll_div fll;
u16 reg, clk1, aif_reg, aif_src;
- unsigned long timeout;
bool was_enabled;
switch (id) {
@@ -2152,9 +2106,6 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
return -EINVAL;
}
- reg = snd_soc_component_read32(component, WM8994_FLL1_CONTROL_1 + reg_offset);
- was_enabled = reg & WM8994_FLL1_ENA;
-
switch (src) {
case 0:
/* Allow no source specification when stopping */
@@ -2166,10 +2117,12 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
case WM8994_FLL_SRC_MCLK2:
case WM8994_FLL_SRC_LRCLK:
case WM8994_FLL_SRC_BCLK:
+ src = wm8994_fll_desc[0].clk_ref_map[src - WM8994_FLL_SRC_MCLK1];
break;
case WM8994_FLL_SRC_INTERNAL:
freq_in = 12000000;
freq_out = 12000000;
+ src = FLL_REF_OSC;
break;
default:
return -EINVAL;
@@ -2180,18 +2133,6 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
wm8994->fll[id].in == freq_in && wm8994->fll[id].out == freq_out)
return 0;
- /* If we're stopping the FLL redo the old config - no
- * registers will actually be written but we avoid GCC flow
- * analysis bugs spewing warnings.
- */
- if (freq_out)
- ret = wm8994_get_fll_config(control, &fll, freq_in, freq_out);
- else
- ret = wm8994_get_fll_config(control, &fll, wm8994->fll[id].in,
- wm8994->fll[id].out);
- if (ret < 0)
- return ret;
-
/* Make sure that we're not providing SYSCLK right now */
clk1 = snd_soc_component_read32(component, WM8994_CLOCKING_1);
if (clk1 & WM8994_SYSCLK_SRC)
@@ -2207,9 +2148,11 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
return -EBUSY;
}
- /* We always need to disable the FLL while reconfiguring */
- snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_1 + reg_offset,
- WM8994_FLL1_ENA, 0);
+ was_enabled = wm_fll_is_enabled(&wm8994->fll_hw[id]) > 0;
+
+ ret = wm_fll_disable(&wm8994->fll_hw[id]);
+ if (ret)
+ return ret;
if (wm8994->fll_byp && src == WM8994_FLL_SRC_BCLK &&
freq_in == freq_out && freq_out) {
@@ -2217,46 +2160,21 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_5 + reg_offset,
WM8958_FLL1_BYP, WM8958_FLL1_BYP);
goto out;
+ } else if (wm8994->fll_byp) {
+ snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_5 + reg_offset,
+ WM8958_FLL1_BYP, 0);
}
- reg = (fll.outdiv << WM8994_FLL1_OUTDIV_SHIFT) |
- (fll.fll_fratio << WM8994_FLL1_FRATIO_SHIFT);
- snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_2 + reg_offset,
- WM8994_FLL1_OUTDIV_MASK |
- WM8994_FLL1_FRATIO_MASK, reg);
-
- snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_3 + reg_offset,
- WM8994_FLL1_K_MASK, fll.k);
-
- snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_4 + reg_offset,
- WM8994_FLL1_N_MASK,
- fll.n << WM8994_FLL1_N_SHIFT);
-
- if (fll.lambda) {
- snd_soc_component_update_bits(component, WM8958_FLL1_EFS_1 + reg_offset,
- WM8958_FLL1_LAMBDA_MASK,
- fll.lambda);
- snd_soc_component_update_bits(component, WM8958_FLL1_EFS_2 + reg_offset,
- WM8958_FLL1_EFS_ENA, WM8958_FLL1_EFS_ENA);
- } else {
- snd_soc_component_update_bits(component, WM8958_FLL1_EFS_2 + reg_offset,
- WM8958_FLL1_EFS_ENA, 0);
- }
-
- snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_5 + reg_offset,
- WM8994_FLL1_FRC_NCO | WM8958_FLL1_BYP |
- WM8994_FLL1_REFCLK_DIV_MASK |
- WM8994_FLL1_REFCLK_SRC_MASK,
- ((src == WM8994_FLL_SRC_INTERNAL)
- << WM8994_FLL1_FRC_NCO_SHIFT) |
- (fll.clk_ref_div << WM8994_FLL1_REFCLK_DIV_SHIFT) |
- (src - 1));
-
- /* Clear any pending completion from a previous failure */
- try_wait_for_completion(&wm8994->fll_locked[id]);
-
- /* Enable (with fractional mode if required) */
if (freq_out) {
+ wm8994->fll_hw[id].freq_in = freq_in;
+ ret = wm_fll_set_parent(&wm8994->fll_hw[id], src);
+ if (!ret)
+ ret = wm_fll_set_rate(&wm8994->fll_hw[id], freq_out);
+ if (!ret)
+ ret = wm_fll_enable(&wm8994->fll_hw[id]);
+ if (ret < 0)
+ return ret;
+
/* Enable VMID if we need it */
if (!was_enabled) {
active_reference(component);
@@ -2273,27 +2191,6 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
break;
}
}
-
- reg = WM8994_FLL1_ENA;
-
- if (fll.k)
- reg |= WM8994_FLL1_FRAC;
- if (src == WM8994_FLL_SRC_INTERNAL)
- reg |= WM8994_FLL1_OSC_ENA;
-
- snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_1 + reg_offset,
- WM8994_FLL1_ENA | WM8994_FLL1_OSC_ENA |
- WM8994_FLL1_FRAC, reg);
-
- if (wm8994->fll_locked_irq) {
- timeout = wait_for_completion_timeout(&wm8994->fll_locked[id],
- msecs_to_jiffies(10));
- if (timeout == 0)
- dev_warn(component->dev,
- "Timed out waiting for FLL lock\n");
- } else {
- msleep(5);
- }
} else {
if (was_enabled) {
switch (control->type) {
@@ -2350,15 +2247,6 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
return 0;
}
-static irqreturn_t wm8994_fll_locked_irq(int irq, void *data)
-{
- struct completion *completion = data;
-
- complete(completion);
-
- return IRQ_HANDLED;
-}
-
static int opclk_divs[] = { 10, 20, 30, 40, 55, 60, 80, 120, 160 };
static int wm8994_set_fll(struct snd_soc_dai *dai, int id, int src,
@@ -3992,6 +3880,18 @@ static int wm8994_component_probe(struct snd_soc_component *component)
snd_soc_component_init_regmap(component, control->regmap);
+ for (i = 0; i < ARRAY_SIZE(wm8994->fll_hw); ++i) {
+ wm8994->fll_hw[i].regmap = control->regmap;
+ if (control->type == WM8994)
+ wm8994->fll_hw[i].desc = wm8994_fll_desc;
+ else
+ wm8994->fll_hw[i].desc = wm8958_fll_desc;
+
+ ret = wm_fll_init(&wm8994->fll_hw[i]);
+ if (ret)
+ return ret;
+ }
+
wm8994->hubs.component = component;
mutex_init(&wm8994->accdet_lock);
@@ -4013,9 +3913,6 @@ static int wm8994_component_probe(struct snd_soc_component *component)
INIT_DELAYED_WORK(&wm8994->mic_complete_work, wm8958_mic_work);
- for (i = 0; i < ARRAY_SIZE(wm8994->fll_locked); i++)
- init_completion(&wm8994->fll_locked[i]);
-
wm8994->micdet_irq = control->pdata.micdet_irq;
/* By default use idle_bias_off, will override for WM8994 */
@@ -4166,16 +4063,6 @@ static int wm8994_component_probe(struct snd_soc_component *component)
break;
}
- wm8994->fll_locked_irq = true;
- for (i = 0; i < ARRAY_SIZE(wm8994->fll_locked); i++) {
- ret = wm8994_request_irq(wm8994->wm8994,
- WM8994_IRQ_FLL1_LOCK + i,
- wm8994_fll_locked_irq, "FLL lock",
- &wm8994->fll_locked[i]);
- if (ret != 0)
- wm8994->fll_locked_irq = false;
- }
-
/* Make sure we can read from the GPIOs if they're inputs */
pm_runtime_get_sync(component->dev);
@@ -4377,9 +4264,6 @@ static int wm8994_component_probe(struct snd_soc_component *component)
wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_MIC1_SHRT, wm8994);
if (wm8994->micdet_irq)
free_irq(wm8994->micdet_irq, wm8994);
- for (i = 0; i < ARRAY_SIZE(wm8994->fll_locked); i++)
- wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_FLL1_LOCK + i,
- &wm8994->fll_locked[i]);
wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_DCS_DONE,
&wm8994->hubs);
wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_FIFOS_ERR, component);
@@ -4393,11 +4277,6 @@ static void wm8994_component_remove(struct snd_soc_component *component)
{
struct wm8994_priv *wm8994 = snd_soc_component_get_drvdata(component);
struct wm8994 *control = wm8994->wm8994;
- int i;
-
- for (i = 0; i < ARRAY_SIZE(wm8994->fll_locked); i++)
- wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_FLL1_LOCK + i,
- &wm8994->fll_locked[i]);
wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_DCS_DONE,
&wm8994->hubs);
diff --git a/sound/soc/codecs/wm8994.h b/sound/soc/codecs/wm8994.h
index 1d6f2abe1c11..9c61d95c9053 100644
--- a/sound/soc/codecs/wm8994.h
+++ b/sound/soc/codecs/wm8994.h
@@ -13,6 +13,7 @@
#include <linux/mutex.h>
#include "wm_hubs.h"
+#include "wm_fll.h"
/* Sources for AIF1/2 SYSCLK - use with set_dai_sysclk() */
#define WM8994_SYSCLK_MCLK1 1
@@ -80,8 +81,7 @@ struct wm8994_priv {
int aifdiv[2];
int channels[2];
struct wm8994_fll_config fll[2], fll_suspend[2];
- struct completion fll_locked[2];
- bool fll_locked_irq;
+ struct wm_fll_data fll_hw[2];
bool fll_byp;
bool clk_has_run;
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v2 3/4] ASoC: wm8904: automatically choose clock source
From: Michał Mirosław @ 2019-08-25 12:17 UTC (permalink / raw)
To: alsa-devel, patches
Cc: Kate Stewart, Maxime Jourdan, Alexandre Belloni,
Kuninori Morimoto, Kirill Marinushkin, Takashi Iwai,
Paul Cercueil, Srinivas Kandagatla, Jerome Brunet, Anders Roxell,
Ludovic Desroches, linux-arm-kernel, Codrin Ciubotariu,
Charles Keepax, Liam Girdwood, Piotr Stankiewicz,
Annaliese McDermond, Richard Fitzgerald, Mark Brown,
Nariman Poushin, Thomas Gleixner, Jaroslav Kysela, zhong jiang,
Allison Randal, Greg Kroah-Hartman, Randy Dunlap, Nikesh Oswal,
linux-kernel, Enrico Weigelt
In-Reply-To: <cover.1566734630.git.mirq-linux@rere.qmqm.pl>
Choose clock source automatically if not provided. This will be the case
with eg. audio-graph-card.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
sound/soc/codecs/wm8904.c | 42 +++++++++++++++++++++++++++++++++++++--
1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
index c9318fe34f91..946315d4cecf 100644
--- a/sound/soc/codecs/wm8904.c
+++ b/sound/soc/codecs/wm8904.c
@@ -367,15 +367,34 @@ static int wm8904_enable_sysclk(struct wm8904_priv *priv)
return err;
}
+static int wm8904_bump_fll_sysclk(unsigned int *rate);
+
static int wm8904_set_sysclk(struct snd_soc_dai *dai, int clk_id,
unsigned int rate, int dir)
{
struct snd_soc_component *component = dai->component;
struct wm8904_priv *wm8904 = snd_soc_component_get_drvdata(component);
unsigned int clock0, clock2;
- int err;
+ int err, do_div = false;
switch (clk_id) {
+ case 0:
+ if (rate == clk_round_rate(wm8904->mclk, rate)) {
+ clk_id = WM8904_CLK_MCLK;
+ } else if (rate * 2 == clk_round_rate(wm8904->mclk, rate * 2)) {
+ rate *= 2;
+ clk_id = WM8904_CLK_MCLK;
+ do_div = true;
+ } else {
+ clk_id = WM8904_CLK_FLL;
+ err = wm8904_bump_fll_sysclk(&rate);
+ if (err) {
+ dev_dbg(component->dev, "Can't match %u over FLL 1406250 Hz minimum\n", rate);
+ return err;
+ }
+ }
+ break;
+
case WM8904_CLK_MCLK:
case WM8904_CLK_FLL:
break;
@@ -421,7 +440,9 @@ static int wm8904_set_sysclk(struct snd_soc_dai *dai, int clk_id,
}
/* SYSCLK shouldn't be over 13.5MHz */
- if (rate > 13500000) {
+ if (rate > 13500000)
+ do_div = true;
+ if (do_div) {
clock0 = WM8904_MCLK_DIV;
wm8904->sysclk_rate = rate / 2;
} else {
@@ -1350,6 +1371,23 @@ static struct {
{ 480, 20 },
};
+static int wm8904_bump_fll_sysclk(unsigned int *rate)
+{
+ int i;
+
+ /* bump SYSCLK rate if below minimal FLL output */
+
+ for (i = 0; i < ARRAY_SIZE(bclk_divs); i++) {
+ if (*rate * bclk_divs[i].div >= 1406250 * 10)
+ break;
+ }
+
+ if (i == ARRAY_SIZE(bclk_divs))
+ return -ERANGE;
+
+ *rate = (*rate * bclk_divs[i].div) / 10;
+ return 0;
+}
static int wm8904_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v2 1/4] ASoC: wm_fll: extract common code for Wolfson FLLs
From: Michał Mirosław @ 2019-08-25 12:17 UTC (permalink / raw)
To: alsa-devel, patches
Cc: Maxime Jourdan, Kate Stewart, Alexandre Belloni,
Kuninori Morimoto, Kirill Marinushkin, Liam Girdwood,
Paul Cercueil, Srinivas Kandagatla, Jerome Brunet, Anders Roxell,
Ludovic Desroches, linux-arm-kernel, Codrin Ciubotariu,
Charles Keepax, Piotr Stankiewicz, Annaliese McDermond,
Richard Fitzgerald, Mark Brown, Nariman Poushin, Thomas Gleixner,
Jaroslav Kysela, zhong jiang, Allison Randal, Greg Kroah-Hartman,
Randy Dunlap, Nikesh Oswal, Takashi Iwai, linux-kernel,
Enrico Weigelt
In-Reply-To: <cover.1566734630.git.mirq-linux@rere.qmqm.pl>
A new implementation for FLLs as contained in WM8904, WM8994 and a few
other Cirrus Logic (formerly Wolfson) codecs. Patches using this common
code follow.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
sound/soc/codecs/Kconfig | 6 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/wm_fll.c | 518 ++++++++++++++++++++++++++++++++++++++
sound/soc/codecs/wm_fll.h | 60 +++++
4 files changed, 586 insertions(+)
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 9f89a5346299..04086acf6d93 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -281,6 +281,12 @@ config SND_SOC_ARIZONA
default m if SND_SOC_WM8997=m
default m if SND_SOC_WM8998=m
+config SND_SOC_WM_FLL
+ tristate
+
+config SND_SOC_WM_FLL_EFS
+ bool
+
config SND_SOC_WM_HUBS
tristate
default y if SND_SOC_WM8993=y || SND_SOC_WM8994=y
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 5b4bb8cf4325..22704fbb7497 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -269,6 +269,7 @@ snd-soc-wm9090-objs := wm9090.o
snd-soc-wm9705-objs := wm9705.o
snd-soc-wm9712-objs := wm9712.o
snd-soc-wm9713-objs := wm9713.o
+snd-soc-wm-fll-objs := wm_fll.o
snd-soc-wm-hubs-objs := wm_hubs.o
snd-soc-zx-aud96p22-objs := zx_aud96p22.o
# Amp
@@ -549,6 +550,7 @@ obj-$(CONFIG_SND_SOC_WM9705) += snd-soc-wm9705.o
obj-$(CONFIG_SND_SOC_WM9712) += snd-soc-wm9712.o
obj-$(CONFIG_SND_SOC_WM9713) += snd-soc-wm9713.o
obj-$(CONFIG_SND_SOC_WM_ADSP) += snd-soc-wm-adsp.o
+obj-$(CONFIG_SND_SOC_WM_FLL) += snd-soc-wm-fll.o
obj-$(CONFIG_SND_SOC_WM_HUBS) += snd-soc-wm-hubs.o
obj-$(CONFIG_SND_SOC_ZX_AUD96P22) += snd-soc-zx-aud96p22.o
diff --git a/sound/soc/codecs/wm_fll.c b/sound/soc/codecs/wm_fll.c
new file mode 100644
index 000000000000..0d8217287030
--- /dev/null
+++ b/sound/soc/codecs/wm_fll.c
@@ -0,0 +1,518 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * wm_fll.c -- WM89xx FLL support
+ *
+ * Copyright 2019 Michał Mirosław
+ *
+ * WM can generate its clock directly from MCLK, from
+ * internal FLL synchronizing to one of hw frame clocks
+ * or from FLL's VCO in free-running mode
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/gcd.h>
+#include <linux/kernel.h>
+
+#include "wm_fll.h"
+
+/* FLL Control 1 */
+#define WM_FLL_CONTROL_1 (hw->desc->ctl_offset + 0)
+#define WM_FLL_FRACN_ENA BIT(2)
+#define WM_FLL_OSC_ENA BIT(1)
+#define WM_FLL_ENA BIT(0)
+
+/* FLL Control 2 */
+#define WM_FLL_CONTROL_2 (hw->desc->ctl_offset + 1)
+#define WM_FLL_OUTDIV GENMASK(13, 8)
+#define WM_FLL_CTRL_RATE GENMASK(6, 4)
+#define WM_FLL_FRATIO GENMASK(2, 0)
+
+/* FLL Control 3 */
+#define WM_FLL_CONTROL_3 (hw->desc->ctl_offset + 2)
+#define WM_FLL_K GENMASK(15, 0)
+
+/* FLL Control 4 */
+#define WM_FLL_CONTROL_4 (hw->desc->ctl_offset + 3)
+#define WM_FLL_N GENMASK(14, 5)
+#define WM_FLL_GAIN GENMASK(3, 0)
+
+/* FLL Control 5 */
+#define WM_FLL_CONTROL_5 (hw->desc->ctl_offset + 4)
+#define WM_FLL_CLK_REF_DIV GENMASK(4, 3)
+#define WM_FLL_CLK_REF_SRC GENMASK(1, 0)
+
+/* Interrupt Status */
+#define WM_INTERRUPT_STATUS (hw->desc->int_offset + 0)
+
+/* FLL NCO Test 0 (part of FLL Control 5 on some chips) */
+#define WM_FLL_NCO_TEST_0 (hw->desc->nco_reg0)
+#define WM_FLL_FRC_NCO BIT(0)
+
+/* FLL NCO Test 1 (part of FLL Control 5 on some chips) */
+#define WM_FLL_NCO_TEST_1 (hw->desc->nco_reg1)
+#define WM_FLL_FRC_NCO_VAL GENMASK(5, 0)
+
+/* FLL EFS 1 (chips with N+THETA/LAMBDA instead of N.K multiplier) */
+#define WM_FLL_EFS_1 (hw->desc->efs_offset + 0)
+#define WM_FLL_LAMBDA GENMASK(15, 0)
+
+/* FLL EFS 2 (chips with N+THETA/LAMBDA instead of N.K multiplier) */
+#define WM_FLL_EFS_2 (hw->desc->efs_offset + 1)
+#define WM_FLL_EFS_ENA BIT(0)
+
+
+/* feature tests */
+#define WM_FLL_USES_EFS(hw) (IS_ENABLED(CONFIG_SND_SOC_WM_FLL_EFS) && hw->desc->efs_offset)
+
+
+static bool wm_fll_in_free_running_mode(struct wm_fll_data *hw)
+{
+ unsigned int val;
+
+ if (!hw->desc->nco_reg0)
+ return false;
+ if (regmap_read(hw->regmap, WM_FLL_NCO_TEST_0, &val) < 0)
+ return false;
+
+ val >>= hw->desc->frc_nco_shift;
+ return FIELD_GET(WM_FLL_FRC_NCO, val);
+}
+
+static int wm_fll_set_free_running_mode(struct wm_fll_data *hw, bool enable)
+{
+ unsigned int val, mask;
+ int err;
+
+ if (!hw->desc->nco_reg0)
+ return enable ? -EINVAL : 0;
+
+ if (enable) {
+ /* set osc freq (approx 96MHz) */
+ val = FIELD_PREP(WM_FLL_FRC_NCO_VAL, 0x19);
+ mask = WM_FLL_FRC_NCO_VAL;
+
+ val <<= hw->desc->frc_nco_val_shift;
+ mask <<= hw->desc->frc_nco_val_shift;
+
+ err = regmap_update_bits(hw->regmap, WM_FLL_NCO_TEST_1,
+ mask, val);
+ if (err)
+ return err;
+ }
+
+ /* set free-running mode */
+ val = FIELD_PREP(WM_FLL_FRC_NCO, enable);
+ mask = WM_FLL_FRC_NCO;
+
+ val <<= hw->desc->frc_nco_shift;
+ mask <<= hw->desc->frc_nco_shift;
+
+ return regmap_update_bits(hw->regmap, WM_FLL_NCO_TEST_0,
+ mask, val);
+}
+
+static int wm_fll_get_parent(struct wm_fll_data *hw)
+{
+ unsigned int val;
+ int err;
+
+ /* free-running mode? */
+ if (wm_fll_in_free_running_mode(hw))
+ return FLL_REF_OSC;
+
+ err = regmap_read(hw->regmap, WM_FLL_CONTROL_5, &val);
+ if (err < 0)
+ return err;
+
+ val = FIELD_GET(WM_FLL_CLK_REF_SRC, val);
+ return hw->desc->clk_ref_map[val];
+}
+
+/**
+ * wm_fll_set_parent() - Change FLL clock source
+ *
+ * @hw: FLL hardware info
+ * @index: FLL source clock id
+ *
+ * Configures FLL for using @index clock as input.
+ *
+ * Return 0 if successful, error code if not.
+ */
+int wm_fll_set_parent(struct wm_fll_data *hw, enum wm_fll_ref_source index)
+{
+ unsigned int ref;
+ bool osc_en;
+ int err;
+
+ osc_en = index == FLL_REF_OSC;
+ err = wm_fll_set_free_running_mode(hw, osc_en);
+ if (osc_en || err)
+ return err;
+
+ err = -EINVAL;
+ for (ref = 0; ref < ARRAY_SIZE(hw->desc->clk_ref_map); ++ref) {
+ if (hw->desc->clk_ref_map[ref] != index)
+ continue;
+
+ err = 0;
+ break;
+ }
+ if (err < 0)
+ return err;
+
+ /* set FLL reference input */
+ ref = FIELD_PREP(WM_FLL_CLK_REF_SRC, ref);
+ err = regmap_update_bits(hw->regmap, WM_FLL_CONTROL_5,
+ WM_FLL_CLK_REF_SRC, ref);
+ return err;
+}
+EXPORT_SYMBOL_GPL(wm_fll_set_parent);
+
+/**
+ * wm_fll_enable() - Enable FLL
+ *
+ * @hw: initialized FLL hardware info
+ *
+ * Requests source clock and starts the FLL.
+ * Waits for lock before returning.
+ *
+ * Return 0 if successful, error code if not.
+ */
+int wm_fll_enable(struct wm_fll_data *hw)
+{
+ unsigned int val;
+ int clk_src;
+ int err, retry;
+
+ err = clk_src = wm_fll_get_parent(hw);
+ if (err < 0)
+ return err;
+
+ if (clk_src == FLL_REF_MCLK) {
+ err = clk_prepare_enable(hw->mclk);
+ if (err < 0)
+ return err;
+ }
+
+ err = regmap_update_bits(hw->regmap, WM_FLL_CONTROL_1,
+ WM_FLL_OSC_ENA, WM_FLL_OSC_ENA);
+ if (err < 0)
+ goto err_out;
+
+ err = regmap_write(hw->regmap, WM_INTERRUPT_STATUS,
+ hw->desc->int_mask);
+ if (err < 0)
+ goto err_out;
+
+ err = regmap_update_bits(hw->regmap, WM_FLL_CONTROL_1,
+ WM_FLL_ENA, WM_FLL_ENA);
+ if (err)
+ goto err_out;
+
+ if (clk_src == FLL_REF_OSC) {
+ usleep_range(150, 250);
+ return 0;
+ }
+
+ for (retry = 3; retry; --retry) {
+ msleep(1);
+ err = regmap_read(hw->regmap, WM_INTERRUPT_STATUS, &val);
+ if (err < 0)
+ goto err_out;
+
+ if (val & hw->desc->int_mask)
+ break;
+ }
+
+ /* it seems that FLL_LOCK might never be asserted */
+ /* eg. WM8904's FLL doesn't, but works anyway */
+ return 0;
+
+err_out:
+ wm_fll_disable(hw);
+
+ if (clk_src == FLL_REF_MCLK)
+ clk_disable_unprepare(hw->mclk);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(wm_fll_enable);
+
+/**
+ * wm_fll_disable() - Disable FLL
+ *
+ * @hw: initialized FLL hardware info
+ *
+ * Return 0 if successful, error code if not.
+ */
+int wm_fll_disable(struct wm_fll_data *hw)
+{
+ return regmap_update_bits(hw->regmap, WM_FLL_CONTROL_1,
+ WM_FLL_ENA|WM_FLL_OSC_ENA,
+ 0);
+}
+EXPORT_SYMBOL_GPL(wm_fll_disable);
+
+/**
+ * wm_fll_is_enabled() - Check whether FLL is enabled
+ *
+ * @hw: initialized FLL hardware info
+ *
+ * Returns 0 if disabled, 1 if enabled, or negative error code.
+ */
+int wm_fll_is_enabled(struct wm_fll_data *hw)
+{
+ unsigned int val;
+ int err;
+
+ err = regmap_read(hw->regmap, WM_FLL_CONTROL_1, &val);
+ if (err < 0)
+ return err;
+
+ return FIELD_GET(WM_FLL_ENA, val);
+}
+EXPORT_SYMBOL_GPL(wm_fll_is_enabled);
+
+static unsigned int wm_fll_apply_refdiv(unsigned long *parent_rate)
+{
+ unsigned int refdiv;
+
+ /* FLL input divider; should ensure Fin <= 13.5MHz */
+
+ refdiv = DIV_ROUND_UP(*parent_rate, 13500000);
+ refdiv = order_base_2(refdiv);
+ if (refdiv > 3)
+ refdiv = 3;
+
+ *parent_rate >>= refdiv;
+
+ return refdiv;
+}
+
+static unsigned int wm_fll_apply_fratio(unsigned long *parent_rate)
+{
+ unsigned int fratio;
+
+ /* FLL comparator divider; efectively Fin multiplier */
+ /* as tabularized in WM datasheet */
+
+ if (*parent_rate >= 256000)
+ fratio = *parent_rate < 1024000;
+ else if (*parent_rate >= 64000)
+ fratio = 2 + (*parent_rate < 128000);
+ else
+ fratio = 4;
+
+ *parent_rate <<= fratio;
+
+ return fratio;
+}
+
+static unsigned int wm_fll_apply_outdiv_rev(unsigned long *rate)
+{
+ unsigned int div;
+
+ /* Fvco -> Fout divider; target: 90 <= Fvco <= 100 MHz */
+
+ div = DIV_ROUND_UP(90000000, *rate);
+ if (div > 64) {
+ *rate = 90000000;
+ return 64;
+ }
+
+ if (div < 4)
+ div = 4;
+
+ *rate *= div;
+ return div;
+}
+
+static unsigned long wm_fll_apply_frac(struct wm_fll_data *hw,
+ unsigned long rate_in, unsigned long *rate_out,
+ unsigned int *kdiv_out)
+{
+ unsigned long long freq;
+ unsigned long rate = *rate_out;
+ unsigned int kdiv = 0x10000;
+
+ if (WM_FLL_USES_EFS(hw)) {
+ unsigned int cd, num;
+
+ cd = gcd(rate, rate_in);
+ freq = rate / rate_in;
+ num = rate - freq * rate_in;
+ num /= cd;
+ kdiv = rate_in / cd;
+
+ rate = freq * rate_in + num * rate_in / kdiv;
+ freq = (freq << 16) | num;
+ } else {
+ freq = (unsigned long long)rate << 16;
+ freq += rate_in / 2;
+ do_div(freq, rate_in);
+
+ rate = (freq * rate_in) >> 16;
+ }
+
+ *rate_out = rate;
+ *kdiv_out = kdiv;
+ return freq;
+}
+
+/**
+ * wm_fll_set_rate() - Configures FLL for specified bitrate
+ *
+ * @hw: initialized FLL hardware info
+ * @rate: bitrate to configure for
+ *
+ * Return 0 if successful, error code if not. FLL must be disabled
+ * on entry.
+ */
+int wm_fll_set_rate(struct wm_fll_data *hw, unsigned long rate)
+{
+ unsigned long freq, mclk_rate;
+ unsigned int val, mask, refdiv, outdiv, fratio, kdiv;
+ int err;
+
+ err = wm_fll_is_enabled(hw);
+ if (err < 0)
+ return err;
+ if (err > 0)
+ return -EBUSY;
+
+ err = wm_fll_get_parent(hw);
+ if (err < 0)
+ return err;
+
+ if (err != FLL_REF_OSC) {
+ unsigned long parent_rate;
+
+ if (hw->mclk)
+ hw->freq_in = clk_get_rate(hw->mclk);
+
+ parent_rate = mclk_rate = hw->freq_in;
+ refdiv = wm_fll_apply_refdiv(&parent_rate);
+ fratio = wm_fll_apply_fratio(&parent_rate);
+ outdiv = wm_fll_apply_outdiv_rev(&rate);
+ freq = wm_fll_apply_frac(hw, parent_rate, &rate, &kdiv);
+ } else {
+ unsigned long vco_rate = 96000000;
+
+ mclk_rate = 0;
+ fratio = refdiv = 0;
+ rate = DIV_ROUND_CLOSEST(vco_rate, rate);
+ outdiv = clamp_t(unsigned long, rate, 4, 64);
+ freq = 0x177 << 16;
+ kdiv = 0;
+
+ rate = vco_rate;
+ }
+
+ /* configure */
+
+ dev_dbg(regmap_get_device(hw->regmap),
+ "configuring FLL for %luHz -> %luHz -> %luHz\n",
+ mclk_rate, rate, rate / outdiv);
+ dev_dbg(regmap_get_device(hw->regmap),
+ "FLL settings: N=%lu K=%lu/%u FRATIO=%u OUTDIV=%u REF_DIV=%u\n",
+ freq >> 16, freq & 0xFFFF, kdiv, fratio, outdiv, refdiv);
+
+ val = FIELD_PREP(WM_FLL_CLK_REF_DIV, refdiv);
+ err = regmap_update_bits(hw->regmap, WM_FLL_CONTROL_5,
+ WM_FLL_CLK_REF_DIV, val);
+ if (err < 0)
+ return err;
+
+ val = FIELD_PREP(WM_FLL_OUTDIV, outdiv - 1) |
+ FIELD_PREP(WM_FLL_FRATIO, fratio);
+ mask = WM_FLL_OUTDIV | WM_FLL_FRATIO;
+ err = regmap_update_bits(hw->regmap, WM_FLL_CONTROL_2, mask, val);
+ if (err < 0)
+ return err;
+
+ err = regmap_write(hw->regmap, WM_FLL_CONTROL_3, (uint16_t)freq);
+ if (err < 0)
+ return err;
+
+ if (WM_FLL_USES_EFS(hw)) {
+ val = FIELD_PREP(WM_FLL_EFS_ENA, !!(uint16_t)freq);
+ err = regmap_update_bits(hw->regmap, WM_FLL_EFS_2,
+ WM_FLL_EFS_ENA, val);
+ if (err < 0)
+ return err;
+
+ val = FIELD_PREP(WM_FLL_LAMBDA, kdiv);
+ err = regmap_update_bits(hw->regmap, WM_FLL_EFS_1,
+ WM_FLL_LAMBDA, val);
+ } else {
+ val = FIELD_PREP(WM_FLL_FRACN_ENA, !!(uint16_t)freq);
+ err = regmap_update_bits(hw->regmap, WM_FLL_CONTROL_1,
+ WM_FLL_FRACN_ENA, val);
+ }
+ if (err < 0)
+ return err;
+
+ val = FIELD_PREP(WM_FLL_N, freq >> 16);
+ err = regmap_update_bits(hw->regmap, WM_FLL_CONTROL_4,
+ WM_FLL_N, val);
+ return err;
+}
+EXPORT_SYMBOL_GPL(wm_fll_set_rate);
+
+/**
+ * wm_fll_init() - Initialize FLL
+ *
+ * @hw: FLL hardware info
+ *
+ * Checks and initializes FLL structure.
+ * Requires hw->desc and hw->regmap to be filled in by caller.
+ *
+ * Return 0 if successful, negative error code if not.
+ * A message is logged on error.
+ */
+int wm_fll_init(struct wm_fll_data *hw)
+{
+ if (!IS_ENABLED(CONFIG_SND_SOC_WM_FLL_EFS) && hw->desc->efs_offset) {
+ struct device *dev = regmap_get_device(hw->regmap);
+
+ dev_err(dev, "FLL EFS support not compiled in\n");
+ return -ENOSYS;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(wm_fll_init);
+
+/**
+ * wm_fll_init_with_clk() - Initialize FLL
+ *
+ * @hw: FLL hardware info
+ *
+ * Checks and initializes FLL described in @hw, and requests MCLK input clock.
+ * Requires hw->desc and hw->regmap to be filled in by caller.
+ *
+ * Return 0 if successful, negative error code if not.
+ * A message is logged on error.
+ */
+int wm_fll_init_with_clk(struct wm_fll_data *hw)
+{
+ struct device *dev = regmap_get_device(hw->regmap);
+ int err;
+
+ err = wm_fll_init(hw);
+ if (err)
+ return err;
+
+ hw->mclk = devm_clk_get(dev, "mclk");
+ if (IS_ERR(hw->mclk)) {
+ err = PTR_ERR(hw->mclk);
+ dev_err(dev, "Failed to get MCLK for FLL @0x%x: %d\n",
+ hw->desc->ctl_offset, err);
+ return err;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(wm_fll_init_with_clk);
diff --git a/sound/soc/codecs/wm_fll.h b/sound/soc/codecs/wm_fll.h
new file mode 100644
index 000000000000..8519a8691397
--- /dev/null
+++ b/sound/soc/codecs/wm_fll.h
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * wm_fll.h -- FLL support for Wolfson codecs
+ *
+ * Copyright 2019 Michał Mirosław
+ */
+
+#ifndef _WM_FLL_H
+#define _WM_FLL_H
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/regmap.h>
+
+enum wm_fll_ref_source
+{
+ FLL_REF_MCLK = 1,
+ FLL_REF_MCLK2,
+ FLL_REF_BCLK,
+ FLL_REF_FSCLK,
+ FLL_REF_OSC,
+};
+
+/**
+ * struct wm_fll_desc - FLL variant description
+ *
+ * @offset: FLL control register block offset
+ * @clk_ref_map: FLL_REF_* assignment for each of FLL.REF_SRC field value
+ */
+struct wm_fll_desc
+{
+ uint16_t ctl_offset;
+ uint16_t int_offset;
+ uint16_t int_mask;
+ uint16_t nco_reg0;
+ uint16_t nco_reg1;
+ uint8_t frc_nco_shift;
+ uint8_t frc_nco_val_shift;
+ uint16_t efs_offset;
+ uint8_t clk_ref_map[4];
+};
+
+struct wm_fll_data
+{
+ const struct wm_fll_desc *desc;
+ struct regmap *regmap;
+ unsigned long freq_in;
+
+ struct clk *mclk;
+};
+
+int wm_fll_init(struct wm_fll_data *hw);
+int wm_fll_init_with_clk(struct wm_fll_data *hw);
+int wm_fll_is_enabled(struct wm_fll_data *hw);
+int wm_fll_enable(struct wm_fll_data *hw);
+int wm_fll_disable(struct wm_fll_data *hw);
+int wm_fll_set_parent(struct wm_fll_data *hw, enum wm_fll_ref_source index);
+int wm_fll_set_rate(struct wm_fll_data *hw, unsigned long rate);
+
+#endif /* _WM_FLL_H */
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v2 2/4] ASoC: wm8904: use common FLL code
From: Michał Mirosław @ 2019-08-25 12:17 UTC (permalink / raw)
To: alsa-devel, patches
Cc: Maxime Jourdan, Kate Stewart, Alexandre Belloni,
Kuninori Morimoto, Kirill Marinushkin, Paul Cercueil,
Srinivas Kandagatla, Jerome Brunet, Anders Roxell, Takashi Iwai,
Ludovic Desroches, linux-arm-kernel, Codrin Ciubotariu,
Charles Keepax, Piotr Stankiewicz, Annaliese McDermond,
Richard Fitzgerald, Mark Brown, Nariman Poushin, Thomas Gleixner,
Jaroslav Kysela, zhong jiang, Allison Randal, Greg Kroah-Hartman,
Randy Dunlap, Nikesh Oswal, Liam Girdwood, linux-kernel,
Enrico Weigelt
In-Reply-To: <cover.1566734630.git.mirq-linux@rere.qmqm.pl>
Rework FLL handling to use common code introduced earlier.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
sound/soc/atmel/atmel_wm8904.c | 11 +-
sound/soc/codecs/Kconfig | 1 +
sound/soc/codecs/wm8904.c | 476 ++++++++++-----------------------
sound/soc/codecs/wm8904.h | 5 -
4 files changed, 140 insertions(+), 353 deletions(-)
diff --git a/sound/soc/atmel/atmel_wm8904.c b/sound/soc/atmel/atmel_wm8904.c
index 776b27d3686e..b77ea2495efe 100644
--- a/sound/soc/atmel/atmel_wm8904.c
+++ b/sound/soc/atmel/atmel_wm8904.c
@@ -30,20 +30,11 @@ static int atmel_asoc_wm8904_hw_params(struct snd_pcm_substream *substream,
struct snd_soc_dai *codec_dai = rtd->codec_dai;
int ret;
- ret = snd_soc_dai_set_pll(codec_dai, WM8904_FLL_MCLK, WM8904_FLL_MCLK,
- 32768, params_rate(params) * 256);
- if (ret < 0) {
- pr_err("%s - failed to set wm8904 codec PLL.", __func__);
- return ret;
- }
-
/*
* As here wm8904 use FLL output as its system clock
- * so calling set_sysclk won't care freq parameter
- * then we pass 0
*/
ret = snd_soc_dai_set_sysclk(codec_dai, WM8904_CLK_FLL,
- 0, SND_SOC_CLOCK_IN);
+ params_rate(params) * 256, SND_SOC_CLOCK_IN);
if (ret < 0) {
pr_err("%s -failed to set wm8904 SYSCLK\n", __func__);
return ret;
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 04086acf6d93..1a680023af7d 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -1331,6 +1331,7 @@ config SND_SOC_WM8903
config SND_SOC_WM8904
tristate "Wolfson Microelectronics WM8904 CODEC"
depends on I2C
+ select SND_SOC_WM_FLL
config SND_SOC_WM8940
tristate
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
index bcb3c9d5abf0..c9318fe34f91 100644
--- a/sound/soc/codecs/wm8904.c
+++ b/sound/soc/codecs/wm8904.c
@@ -24,6 +24,7 @@
#include <sound/tlv.h>
#include <sound/wm8904.h>
+#include "wm_fll.h"
#include "wm8904.h"
enum wm8904_type {
@@ -66,12 +67,8 @@ struct wm8904_priv {
int retune_mobile_cfg;
struct soc_enum retune_mobile_enum;
- /* FLL setup */
- int fll_src;
- int fll_fref;
- int fll_fout;
-
/* Clocking configuration */
+ struct wm_fll_data fll;
unsigned int mclk_rate;
int sysclk_src;
unsigned int sysclk_rate;
@@ -311,35 +308,111 @@ static bool wm8904_readable_register(struct device *dev, unsigned int reg)
}
}
-static int wm8904_configure_clocking(struct snd_soc_component *component)
+static void wm8904_unprepare_sysclk(struct wm8904_priv *priv)
{
+ switch (priv->sysclk_src) {
+ case WM8904_CLK_MCLK:
+ clk_disable_unprepare(priv->mclk);
+ break;
+
+ case WM8904_CLK_FLL:
+ wm_fll_disable(&priv->fll);
+ break;
+ }
+}
+
+static int wm8904_prepare_sysclk(struct wm8904_priv *priv)
+{
+ int err;
+
+ switch (priv->sysclk_src) {
+ case WM8904_CLK_MCLK:
+ err = clk_set_rate(priv->mclk, priv->mclk_rate);
+ if (!err)
+ err = clk_prepare_enable(priv->mclk);
+ break;
+
+ case WM8904_CLK_FLL:
+ err = wm_fll_enable(&priv->fll);
+ break;
+
+ default:
+ err = -EINVAL;
+ break;
+ }
+
+ return err;
+}
+
+static void wm8904_disable_sysclk(struct wm8904_priv *priv)
+{
+ regmap_update_bits(priv->regmap, WM8904_CLOCK_RATES_2,
+ WM8904_CLK_SYS_ENA, 0);
+ wm8904_unprepare_sysclk(priv);
+}
+
+static int wm8904_enable_sysclk(struct wm8904_priv *priv)
+{
+ int err;
+
+ err = wm8904_prepare_sysclk(priv);
+ if (err < 0)
+ return err;
+
+ err = regmap_update_bits(priv->regmap, WM8904_CLOCK_RATES_2,
+ WM8904_CLK_SYS_ENA_MASK, WM8904_CLK_SYS_ENA);
+ if (err < 0)
+ wm8904_unprepare_sysclk(priv);
+
+ return err;
+}
+
+static int wm8904_set_sysclk(struct snd_soc_dai *dai, int clk_id,
+ unsigned int rate, int dir)
+{
+ struct snd_soc_component *component = dai->component;
struct wm8904_priv *wm8904 = snd_soc_component_get_drvdata(component);
- unsigned int clock0, clock2, rate;
+ unsigned int clock0, clock2;
+ int err;
+
+ switch (clk_id) {
+ case WM8904_CLK_MCLK:
+ case WM8904_CLK_FLL:
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ if (clk_id == wm8904->sysclk_src && rate == wm8904->mclk_rate)
+ return 0;
+
+ dev_dbg(dai->dev, "Clock source is %d at %uHz\n", clk_id, rate);
/* Gate the clock while we're updating to avoid misclocking */
clock2 = snd_soc_component_read32(component, WM8904_CLOCK_RATES_2);
- snd_soc_component_update_bits(component, WM8904_CLOCK_RATES_2,
- WM8904_SYSCLK_SRC, 0);
+ wm8904_disable_sysclk(wm8904);
+
+ wm8904->sysclk_src = clk_id;
+ wm8904->mclk_rate = rate;
- /* This should be done on init() for bypass paths */
switch (wm8904->sysclk_src) {
case WM8904_CLK_MCLK:
- dev_dbg(component->dev, "Using %dHz MCLK\n", wm8904->mclk_rate);
+ dev_dbg(component->dev, "Using %dHz MCLK\n", rate);
clock2 &= ~WM8904_SYSCLK_SRC;
- rate = wm8904->mclk_rate;
-
- /* Ensure the FLL is stopped */
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_1,
- WM8904_FLL_OSC_ENA | WM8904_FLL_ENA, 0);
break;
case WM8904_CLK_FLL:
- dev_dbg(component->dev, "Using %dHz FLL clock\n",
- wm8904->fll_fout);
+ err = wm_fll_set_rate(&wm8904->fll, rate);
+ if (err < 0) {
+ dev_err(component->dev, "Failed to set FLL rate: %d\n", err);
+ return err;
+ }
+
+ dev_dbg(component->dev, "Using %dHz FLL clock\n", rate);
clock2 |= WM8904_SYSCLK_SRC;
- rate = wm8904->fll_fout;
break;
default:
@@ -356,11 +429,18 @@ static int wm8904_configure_clocking(struct snd_soc_component *component)
wm8904->sysclk_rate = rate;
}
- snd_soc_component_update_bits(component, WM8904_CLOCK_RATES_0, WM8904_MCLK_DIV,
- clock0);
-
+ snd_soc_component_update_bits(component, WM8904_CLOCK_RATES_0,
+ WM8904_MCLK_DIV, clock0);
snd_soc_component_update_bits(component, WM8904_CLOCK_RATES_2,
- WM8904_CLK_SYS_ENA | WM8904_SYSCLK_SRC, clock2);
+ WM8904_SYSCLK_SRC, clock2);
+
+ if (clock2 & WM8904_CLK_SYS_ENA) {
+ err = wm8904_enable_sysclk(wm8904);
+ if (err < 0) {
+ dev_err(component->dev, "Failed to reenable CLK_SYS: %d\n", err);
+ return err;
+ }
+ }
dev_dbg(component->dev, "CLK_SYS is %dHz\n", wm8904->sysclk_rate);
@@ -655,33 +735,21 @@ static int sysclk_event(struct snd_soc_dapm_widget *w,
{
struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
struct wm8904_priv *wm8904 = snd_soc_component_get_drvdata(component);
+ int ret = 0;
switch (event) {
case SND_SOC_DAPM_PRE_PMU:
- /* If we're using the FLL then we only start it when
- * required; we assume that the configuration has been
- * done previously and all we need to do is kick it
- * off.
- */
- switch (wm8904->sysclk_src) {
- case WM8904_CLK_FLL:
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_1,
- WM8904_FLL_OSC_ENA,
- WM8904_FLL_OSC_ENA);
-
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_1,
- WM8904_FLL_ENA,
- WM8904_FLL_ENA);
- break;
-
- default:
- break;
- }
+ ret = wm8904_prepare_sysclk(wm8904);
+ if (ret)
+ dev_err(component->dev,
+ "Failed to prepare SYSCLK: %d\n", ret);
+ else
+ dev_dbg(component->dev, "SYSCLK on\n");
break;
case SND_SOC_DAPM_POST_PMD:
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_1,
- WM8904_FLL_OSC_ENA | WM8904_FLL_ENA, 0);
+ wm8904_unprepare_sysclk(wm8904);
+ dev_dbg(component->dev, "SYSCLK off\n");
break;
}
@@ -1289,7 +1357,7 @@ static int wm8904_hw_params(struct snd_pcm_substream *substream,
{
struct snd_soc_component *component = dai->component;
struct wm8904_priv *wm8904 = snd_soc_component_get_drvdata(component);
- int ret, i, best, best_val, cur_val;
+ int i, best, best_val, cur_val;
unsigned int aif1 = 0;
unsigned int aif2 = 0;
unsigned int aif3 = 0;
@@ -1324,13 +1392,8 @@ static int wm8904_hw_params(struct snd_pcm_substream *substream,
return -EINVAL;
}
-
dev_dbg(component->dev, "Target BCLK is %dHz\n", wm8904->bclk);
- ret = wm8904_configure_clocking(component);
- if (ret != 0)
- return ret;
-
/* Select nearest CLK_SYS_RATE */
best = 0;
best_val = abs((wm8904->sysclk_rate / clk_sys_rates[0].ratio)
@@ -1382,8 +1445,8 @@ static int wm8904_hw_params(struct snd_pcm_substream *substream,
}
}
wm8904->bclk = (wm8904->sysclk_rate * 10) / bclk_divs[best].div;
- dev_dbg(component->dev, "Selected BCLK_DIV of %d for %dHz BCLK\n",
- bclk_divs[best].div, wm8904->bclk);
+ dev_dbg(component->dev, "Selected BCLK_DIV of %d.%d for %dHz BCLK\n",
+ bclk_divs[best].div / 10, bclk_divs[best].div % 10, wm8904->bclk);
aif2 |= bclk_divs[best].bclk_div;
/* LRCLK is a simple fraction of BCLK */
@@ -1410,34 +1473,6 @@ static int wm8904_hw_params(struct snd_pcm_substream *substream,
return 0;
}
-
-static int wm8904_set_sysclk(struct snd_soc_dai *dai, int clk_id,
- unsigned int freq, int dir)
-{
- struct snd_soc_component *component = dai->component;
- struct wm8904_priv *priv = snd_soc_component_get_drvdata(component);
-
- switch (clk_id) {
- case WM8904_CLK_MCLK:
- priv->sysclk_src = clk_id;
- priv->mclk_rate = freq;
- break;
-
- case WM8904_CLK_FLL:
- priv->sysclk_src = clk_id;
- break;
-
- default:
- return -EINVAL;
- }
-
- dev_dbg(dai->dev, "Clock source is %d at %uHz\n", clk_id, freq);
-
- wm8904_configure_clocking(component);
-
- return 0;
-}
-
static int wm8904_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
{
struct snd_soc_component *component = dai->component;
@@ -1577,253 +1612,6 @@ static int wm8904_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
return 0;
}
-struct _fll_div {
- u16 fll_fratio;
- u16 fll_outdiv;
- u16 fll_clk_ref_div;
- u16 n;
- u16 k;
-};
-
-/* The size in bits of the FLL divide multiplied by 10
- * to allow rounding later */
-#define FIXED_FLL_SIZE ((1 << 16) * 10)
-
-static struct {
- unsigned int min;
- unsigned int max;
- u16 fll_fratio;
- int ratio;
-} fll_fratios[] = {
- { 0, 64000, 4, 16 },
- { 64000, 128000, 3, 8 },
- { 128000, 256000, 2, 4 },
- { 256000, 1000000, 1, 2 },
- { 1000000, 13500000, 0, 1 },
-};
-
-static int fll_factors(struct _fll_div *fll_div, unsigned int Fref,
- unsigned int Fout)
-{
- u64 Kpart;
- unsigned int K, Ndiv, Nmod, target;
- unsigned int div;
- int i;
-
- /* Fref must be <=13.5MHz */
- div = 1;
- fll_div->fll_clk_ref_div = 0;
- while ((Fref / div) > 13500000) {
- div *= 2;
- fll_div->fll_clk_ref_div++;
-
- if (div > 8) {
- pr_err("Can't scale %dMHz input down to <=13.5MHz\n",
- Fref);
- return -EINVAL;
- }
- }
-
- pr_debug("Fref=%u Fout=%u\n", Fref, Fout);
-
- /* Apply the division for our remaining calculations */
- Fref /= div;
-
- /* Fvco should be 90-100MHz; don't check the upper bound */
- div = 4;
- while (Fout * div < 90000000) {
- div++;
- if (div > 64) {
- pr_err("Unable to find FLL_OUTDIV for Fout=%uHz\n",
- Fout);
- return -EINVAL;
- }
- }
- target = Fout * div;
- fll_div->fll_outdiv = div - 1;
-
- pr_debug("Fvco=%dHz\n", target);
-
- /* Find an appropriate FLL_FRATIO and factor it out of the target */
- for (i = 0; i < ARRAY_SIZE(fll_fratios); i++) {
- if (fll_fratios[i].min <= Fref && Fref <= fll_fratios[i].max) {
- fll_div->fll_fratio = fll_fratios[i].fll_fratio;
- target /= fll_fratios[i].ratio;
- break;
- }
- }
- if (i == ARRAY_SIZE(fll_fratios)) {
- pr_err("Unable to find FLL_FRATIO for Fref=%uHz\n", Fref);
- return -EINVAL;
- }
-
- /* Now, calculate N.K */
- Ndiv = target / Fref;
-
- fll_div->n = Ndiv;
- Nmod = target % Fref;
- pr_debug("Nmod=%d\n", Nmod);
-
- /* Calculate fractional part - scale up so we can round. */
- Kpart = FIXED_FLL_SIZE * (long long)Nmod;
-
- do_div(Kpart, Fref);
-
- K = Kpart & 0xFFFFFFFF;
-
- if ((K % 10) >= 5)
- K += 5;
-
- /* Move down to proper range now rounding is done */
- fll_div->k = K / 10;
-
- pr_debug("N=%x K=%x FLL_FRATIO=%x FLL_OUTDIV=%x FLL_CLK_REF_DIV=%x\n",
- fll_div->n, fll_div->k,
- fll_div->fll_fratio, fll_div->fll_outdiv,
- fll_div->fll_clk_ref_div);
-
- return 0;
-}
-
-static int wm8904_set_fll(struct snd_soc_dai *dai, int fll_id, int source,
- unsigned int Fref, unsigned int Fout)
-{
- struct snd_soc_component *component = dai->component;
- struct wm8904_priv *wm8904 = snd_soc_component_get_drvdata(component);
- struct _fll_div fll_div;
- int ret, val;
- int clock2, fll1;
-
- /* Any change? */
- if (source == wm8904->fll_src && Fref == wm8904->fll_fref &&
- Fout == wm8904->fll_fout)
- return 0;
-
- clock2 = snd_soc_component_read32(component, WM8904_CLOCK_RATES_2);
-
- if (Fout == 0) {
- dev_dbg(component->dev, "FLL disabled\n");
-
- wm8904->fll_fref = 0;
- wm8904->fll_fout = 0;
-
- /* Gate SYSCLK to avoid glitches */
- snd_soc_component_update_bits(component, WM8904_CLOCK_RATES_2,
- WM8904_CLK_SYS_ENA, 0);
-
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_1,
- WM8904_FLL_OSC_ENA | WM8904_FLL_ENA, 0);
-
- goto out;
- }
-
- /* Validate the FLL ID */
- switch (source) {
- case WM8904_FLL_MCLK:
- case WM8904_FLL_LRCLK:
- case WM8904_FLL_BCLK:
- ret = fll_factors(&fll_div, Fref, Fout);
- if (ret != 0)
- return ret;
- break;
-
- case WM8904_FLL_FREE_RUNNING:
- dev_dbg(component->dev, "Using free running FLL\n");
- /* Force 12MHz and output/4 for now */
- Fout = 12000000;
- Fref = 12000000;
-
- memset(&fll_div, 0, sizeof(fll_div));
- fll_div.fll_outdiv = 3;
- break;
-
- default:
- dev_err(component->dev, "Unknown FLL ID %d\n", fll_id);
- return -EINVAL;
- }
-
- /* Save current state then disable the FLL and SYSCLK to avoid
- * misclocking */
- fll1 = snd_soc_component_read32(component, WM8904_FLL_CONTROL_1);
- snd_soc_component_update_bits(component, WM8904_CLOCK_RATES_2,
- WM8904_CLK_SYS_ENA, 0);
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_1,
- WM8904_FLL_OSC_ENA | WM8904_FLL_ENA, 0);
-
- /* Unlock forced oscilator control to switch it on/off */
- snd_soc_component_update_bits(component, WM8904_CONTROL_INTERFACE_TEST_1,
- WM8904_USER_KEY, WM8904_USER_KEY);
-
- if (fll_id == WM8904_FLL_FREE_RUNNING) {
- val = WM8904_FLL_FRC_NCO;
- } else {
- val = 0;
- }
-
- snd_soc_component_update_bits(component, WM8904_FLL_NCO_TEST_1, WM8904_FLL_FRC_NCO,
- val);
- snd_soc_component_update_bits(component, WM8904_CONTROL_INTERFACE_TEST_1,
- WM8904_USER_KEY, 0);
-
- switch (fll_id) {
- case WM8904_FLL_MCLK:
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_5,
- WM8904_FLL_CLK_REF_SRC_MASK, 0);
- break;
-
- case WM8904_FLL_LRCLK:
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_5,
- WM8904_FLL_CLK_REF_SRC_MASK, 1);
- break;
-
- case WM8904_FLL_BCLK:
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_5,
- WM8904_FLL_CLK_REF_SRC_MASK, 2);
- break;
- }
-
- if (fll_div.k)
- val = WM8904_FLL_FRACN_ENA;
- else
- val = 0;
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_1,
- WM8904_FLL_FRACN_ENA, val);
-
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_2,
- WM8904_FLL_OUTDIV_MASK | WM8904_FLL_FRATIO_MASK,
- (fll_div.fll_outdiv << WM8904_FLL_OUTDIV_SHIFT) |
- (fll_div.fll_fratio << WM8904_FLL_FRATIO_SHIFT));
-
- snd_soc_component_write(component, WM8904_FLL_CONTROL_3, fll_div.k);
-
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_4, WM8904_FLL_N_MASK,
- fll_div.n << WM8904_FLL_N_SHIFT);
-
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_5,
- WM8904_FLL_CLK_REF_DIV_MASK,
- fll_div.fll_clk_ref_div
- << WM8904_FLL_CLK_REF_DIV_SHIFT);
-
- dev_dbg(component->dev, "FLL configured for %dHz->%dHz\n", Fref, Fout);
-
- wm8904->fll_fref = Fref;
- wm8904->fll_fout = Fout;
- wm8904->fll_src = source;
-
- /* Enable the FLL if it was previously active */
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_1,
- WM8904_FLL_OSC_ENA, fll1);
- snd_soc_component_update_bits(component, WM8904_FLL_CONTROL_1,
- WM8904_FLL_ENA, fll1);
-
-out:
- /* Reenable SYSCLK if it was previously active */
- snd_soc_component_update_bits(component, WM8904_CLOCK_RATES_2,
- WM8904_CLK_SYS_ENA, clock2);
-
- return 0;
-}
-
static int wm8904_digital_mute(struct snd_soc_dai *codec_dai, int mute)
{
struct snd_soc_component *component = codec_dai->component;
@@ -1871,15 +1659,6 @@ static int wm8904_set_bias_level(struct snd_soc_component *component,
return ret;
}
- ret = clk_prepare_enable(wm8904->mclk);
- if (ret) {
- dev_err(component->dev,
- "Failed to enable MCLK: %d\n", ret);
- regulator_bulk_disable(ARRAY_SIZE(wm8904->supplies),
- wm8904->supplies);
- return ret;
- }
-
regcache_cache_only(wm8904->regmap, false);
regcache_sync(wm8904->regmap);
@@ -1922,7 +1701,6 @@ static int wm8904_set_bias_level(struct snd_soc_component *component,
regulator_bulk_disable(ARRAY_SIZE(wm8904->supplies),
wm8904->supplies);
- clk_disable_unprepare(wm8904->mclk);
break;
}
return 0;
@@ -1937,7 +1715,6 @@ static const struct snd_soc_dai_ops wm8904_dai_ops = {
.set_sysclk = wm8904_set_sysclk,
.set_fmt = wm8904_set_fmt,
.set_tdm_slot = wm8904_set_tdm_slot,
- .set_pll = wm8904_set_fll,
.hw_params = wm8904_hw_params,
.digital_mute = wm8904_digital_mute,
};
@@ -2123,6 +1900,15 @@ static const struct regmap_config wm8904_regmap = {
.num_reg_defaults = ARRAY_SIZE(wm8904_reg_defaults),
};
+static const struct wm_fll_desc wm8904_fll_desc = {
+ .ctl_offset = WM8904_FLL_CONTROL_1,
+ .int_offset = WM8904_INTERRUPT_STATUS,
+ .int_mask = WM8904_FLL_LOCK_EINT_MASK,
+ .nco_reg0 = WM8904_FLL_NCO_TEST_0,
+ .nco_reg1 = WM8904_FLL_NCO_TEST_1,
+ .clk_ref_map = { FLL_REF_MCLK, FLL_REF_BCLK, FLL_REF_FSCLK, /* reserved */ 0 },
+};
+
#ifdef CONFIG_OF
static const struct of_device_id wm8904_of_match[] = {
{
@@ -2165,6 +1951,19 @@ static int wm8904_i2c_probe(struct i2c_client *i2c,
return ret;
}
+ wm8904->fll.regmap = wm8904->regmap;
+ wm8904->fll.desc = &wm8904_fll_desc;
+ ret = wm_fll_init_with_clk(&wm8904->fll);
+ if (ret)
+ return ret;
+
+ ret = wm_fll_set_parent(&wm8904->fll, FLL_REF_MCLK);
+ if (ret < 0) {
+ dev_err(&i2c->dev, "Failed to select MCLK as FLL input: %d\n",
+ ret);
+ return ret;
+ }
+
if (i2c->dev.of_node) {
const struct of_device_id *match;
@@ -2276,6 +2075,7 @@ static int wm8904_i2c_probe(struct i2c_client *i2c,
WM8904_POBCTRL, 0);
/* Can leave the device powered off until we need it */
+ wm8904_disable_sysclk(wm8904);
regcache_cache_only(wm8904->regmap, true);
regulator_bulk_disable(ARRAY_SIZE(wm8904->supplies), wm8904->supplies);
diff --git a/sound/soc/codecs/wm8904.h b/sound/soc/codecs/wm8904.h
index c1bca52f9927..60af09e0bb15 100644
--- a/sound/soc/codecs/wm8904.h
+++ b/sound/soc/codecs/wm8904.h
@@ -13,11 +13,6 @@
#define WM8904_CLK_MCLK 1
#define WM8904_CLK_FLL 2
-#define WM8904_FLL_MCLK 1
-#define WM8904_FLL_BCLK 2
-#define WM8904_FLL_LRCLK 3
-#define WM8904_FLL_FREE_RUNNING 4
-
/*
* Register values.
*/
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH net-next v2 3/3] net: dsa: mt7530: Add support for port 5
From: René van Dorst @ 2019-08-25 12:48 UTC (permalink / raw)
To: David Miller
Cc: andrew, f.fainelli, frank-w, netdev, sean.wang, linux-mips,
linux-mediatek, john, matthias.bgg, vivien.didelot,
linux-arm-kernel
In-Reply-To: <20190824.161912.1377369658338940538.davem@davemloft.net>
Hi David,
Quoting David Miller <davem@davemloft.net>:
> From: René van Dorst <opensource@vdorst.com>
> Date: Wed, 21 Aug 2019 16:45:47 +0200
>
>> + dev_info(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
>> + val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
>
> This is debugging, at best. Please make this a debugging message or
> remove it entirely.
I change it to a debug message.
If there is nothing else I send a new version with this change also
add the tags ack-by Russell King and tested-by Frank Wunderlich.
Greats,
René
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] arm64: dts: allwinner: a64: pine64-plus: Add PHY regulator delay
From: Jernej Skrabec @ 2019-08-25 13:03 UTC (permalink / raw)
To: mripard, wens
Cc: mark.rutland, devicetree, Ondrej Jirman, linux-kernel,
linux-sunxi, robh+dt, linux-arm-kernel
Depending on kernel and bootloader configuration, it's possible that
Realtek ethernet PHY isn't powered on properly. It needs some time
before it can be used.
Fix that by adding 100ms ramp delay to regulator responsible for
powering PHY.
Fixes: 94dcfdc77fc5 ("arm64: allwinner: pine64-plus: Enable dwmac-sun8i")
Suggested-by: Ondrej Jirman <megous@megous.com>
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-plus.dts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-plus.dts
index 24f1aac366d6..9612a34c1762 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-plus.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-plus.dts
@@ -63,3 +63,7 @@
reg = <1>;
};
};
+
+®_dc1sw {
+ regulator-enable-ramp-delay = <100000>;
+};
--
2.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH net-next v2 0/3] net: dsa: mt7530: Convert to PHYLINK and add support for port 5
From: René van Dorst @ 2019-08-25 13:15 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, Frank Wunderlich, netdev,
Sean Wang, linux-mips, David S . Miller, linux-mediatek,
John Crispin, Matthias Brugger, Vivien Didelot, linux-arm-kernel
In-Reply-To: <20190824222935.GG13294@shell.armlinux.org.uk>
Hi Russell,
Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>:
> On Wed, Aug 21, 2019 at 04:45:44PM +0200, René van Dorst wrote:
>> 1. net: dsa: mt7530: Convert to PHYLINK API
>> This patch converts mt7530 to PHYLINK API.
>> 2. dt-bindings: net: dsa: mt7530: Add support for port 5
>> 3. net: dsa: mt7530: Add support for port 5
>> These 2 patches adding support for port 5 of the switch.
>>
>> v1->v2:
>> * Mostly phylink improvements after review.
>> rfc -> v1:
>> * Mostly phylink improvements after review.
>> * Drop phy isolation patches. Adds no value for now.
>> René van Dorst (3):
>> net: dsa: mt7530: Convert to PHYLINK API
>> dt-bindings: net: dsa: mt7530: Add support for port 5
>> net: dsa: mt7530: Add support for port 5
>>
>> .../devicetree/bindings/net/dsa/mt7530.txt | 218 ++++++++++
>> drivers/net/dsa/mt7530.c | 371 +++++++++++++++---
>> drivers/net/dsa/mt7530.h | 61 ++-
>> 3 files changed, 577 insertions(+), 73 deletions(-)
>
> Having looked through this set of patches, I don't see anything
> from the phylink point of view that concerns me. So, for the
> series from the phylink perspective:
>
> Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
Thanks and thanks for reviewing.
Greats,
René
>
> Thanks.
>
> I did notice a dev_info() in patch 3 that you may like to consider
> whether they should be printed at info level or debug level. You
> may keep my ack on the patch when fixing that.
>
> I haven't considered whether the patch passes davem's style
> requirements for networking code; what I spotted did look like
> the declarations were upside-down christmas tree.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 00/11] ftrace: add support for recording function parameters and return value
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
This series introduces a new ftrace feature CONFIG_FTRACE_FUNC_PROTOTYPE to
record function parameters and return value. It can be enabled/disabled at
runtime by a new trace option "record-funcproto".
To achieve this, we need the function prototype infomation and the location of
parameters. A new tool scripts/ftrace/funcprototype is provided to collect such
necessary infomation and put them into kernel. It walks through the DWARF debug
sections in each object file and output assembly code which defines the function
prototype data. Then the assembly code is built into the original object file.
Here is an example of memremap() function:
.section __funcprotostr, "a"
.P_memremap_0:
.string "offset"
.P_memremap_1:
.string "size"
.P_memremap_2:
.string "flags"
.section __funcproto, "a"
.quad memremap
.byte 0x8
.byte 0x3
.quad .P_memremap_0
.byte 0x8
.byte 0x55
.byte 0x0
.quad .P_memremap_1
.byte 0x8
.byte 0x54
.byte 0x0
.quad .P_memremap_2
.byte 0x8
.byte 0x51
.byte 0x0
Note that currently funcprototype only support global functions. Local functions
can also be supported by using the idea described in recordmcount.pl - temporary
change local functions to global.
The C ABI is arch specific. For arch which supports this feature must
implement a new arch-specific interface arch_fgraph_record_params() and
deliver the return value of function to ftrace core part.
This series only add support for x86_64 platform. Other platforms can be
supported in the future.
Here is an example of the graph trace of function pick_next_task_fair().
Note that we only record the parameter and return value of global
functions.
2) | pick_next_task_fair() {
2) | update_blocked_averages() {
2) 0.765 us | _raw_spin_lock_irqsave(lock=0xffff88807da2b100); /* ret=0x0000000000000082 */
2) 0.944 us | update_rq_clock(rq=0xffff88807da2b100);
2) 0.612 us | __update_load_avg_cfs_rq(now=0x000000251b8516ee, cfs_rq=0xffff8880754f7488); /* ret=0 */
2) 0.654 us | __update_load_avg_se(now=0x000000251b8516ee, cfs_rq=0xffff88807da2b180, se=0xffff88807be2e0d8); /* ret=0 */
2) 0.206 us | __update_load_avg_cfs_rq(now=0x000000251b8516ee, cfs_rq=0xffff88807da2b180); /* ret=0 */
2) | __update_load_avg_cfs_rq(now=0x000000251b8516ee, cfs_rq=0xffff888079b5fb18) {
2) 2.410 us | __accumulate_pelt_segments();
2) 3.103 us | } /* ret=1 */
2) 0.193 us | __update_load_avg_cfs_rq(now=0x000000251b8516ee, cfs_rq=0xffff88807da2b180); /* ret=0 */
2) | update_rt_rq_load_avg(now=0x000000251b8516ee, rq=0xffff88807da2b100, running=0) {
2) 0.258 us | __accumulate_pelt_segments();
2) 1.617 us | } /* ret=1 */
2) | update_dl_rq_load_avg(now=0x000000251b8516ee, rq=0xffff88807da2b100, running=0) {
2) 0.230 us | __accumulate_pelt_segments();
2) 1.511 us | } /* ret=1 */
2) 1.040 us | _raw_spin_unlock_irqrestore(lock=0xffff88807da2b100, flags=0x0000000000000082);
2) + 14.739 us | }
2) | load_balance() {
2) | find_busiest_group() {
2) 0.874 us | update_group_capacity(sd=0xffff88807c1d37d0, cpu=2);
2) 1.761 us | idle_cpu();
2) 0.262 us | idle_cpu();
2) 0.217 us | idle_cpu();
2) 6.338 us | }
2) 8.442 us | }
2) 1.823 us | __msecs_to_jiffies(m=0x00000006); /* ret=0x0000000000000002 */
2) | load_balance() {
2) | find_busiest_group() {
2) 0.434 us | idle_cpu();
2) 0.233 us | idle_cpu();
2) 0.210 us | idle_cpu();
2) 2.308 us | }
2) 2.821 us | }
2) 0.263 us | __msecs_to_jiffies(m=0x00000008); /* ret=0x0000000000000002 */
2) 0.977 us | _raw_spin_lock(lock=0xffff88807da2b100);
2) + 32.262 us | }
The printing rules of each value is:
o For signed value, it is always printed as decimal number.
o For unsigned value,
- For value has size great than 8, it is printed as '{..}'.
- For value has size of 1,2,4,8, it is printed as hexadecimal number.
- If failed to record a parameter, it is printed as '?'.
Changbin Du (11):
ftrace: move recordmcount tools to scripts/ftrace
ftrace: introduce new building tool funcprototype
asm-generic: add generic dwarf definition
ftrace/hash: add private data field
ftrace: create memcache for hash entries
ftrace: process function prototype data in vmlinux and modules
ftrace: prepare arch specific interfaces for function prototype
feature
ftrace: introduce core part of function prototype recording
x86_64: add function prototype recording support
ftrace: add doc for new option record-funcproto
MAINTAINERS: make scripts/ftrace/ maintained
Documentation/trace/ftrace.rst | 6 +
MAINTAINERS | 2 +
arch/arm/kernel/ftrace.c | 2 +-
arch/arm64/kernel/ftrace.c | 2 +-
arch/csky/kernel/ftrace.c | 2 +-
arch/microblaze/kernel/ftrace.c | 2 +-
arch/mips/kernel/ftrace.c | 2 +-
arch/nds32/kernel/ftrace.c | 5 +-
arch/parisc/kernel/ftrace.c | 2 +-
arch/powerpc/kernel/trace/ftrace.c | 2 +-
arch/riscv/kernel/ftrace.c | 2 +-
arch/s390/kernel/ftrace.c | 2 +-
arch/sh/kernel/ftrace.c | 2 +-
arch/sparc/kernel/ftrace.c | 2 +-
arch/x86/Kconfig | 1 +
arch/x86/kernel/ftrace.c | 84 +++-
arch/x86/kernel/ftrace_64.S | 4 +-
include/asm-generic/dwarf.h | 199 +++++++++
include/asm-generic/vmlinux.lds.h | 18 +
include/linux/ftrace.h | 55 ++-
include/linux/module.h | 4 +
kernel/module.c | 25 +-
kernel/trace/Kconfig | 19 +
kernel/trace/fgraph.c | 26 +-
kernel/trace/ftrace.c | 164 +++++++-
kernel/trace/trace.h | 20 +-
kernel/trace/trace_entries.h | 10 +
kernel/trace/trace_functions_graph.c | 108 ++++-
kernel/trace/trace_irqsoff.c | 3 +-
kernel/trace/trace_sched_wakeup.c | 3 +-
scripts/.gitignore | 1 -
scripts/Makefile | 2 +-
scripts/Makefile.build | 28 +-
scripts/ftrace/.gitignore | 6 +
scripts/ftrace/Makefile | 9 +
scripts/ftrace/funcprototype.c | 576 +++++++++++++++++++++++++++
scripts/{ => ftrace}/recordmcount.c | 0
scripts/{ => ftrace}/recordmcount.h | 0
scripts/{ => ftrace}/recordmcount.pl | 0
39 files changed, 1340 insertions(+), 60 deletions(-)
create mode 100644 include/asm-generic/dwarf.h
create mode 100644 scripts/ftrace/.gitignore
create mode 100644 scripts/ftrace/Makefile
create mode 100644 scripts/ftrace/funcprototype.c
rename scripts/{ => ftrace}/recordmcount.c (100%)
rename scripts/{ => ftrace}/recordmcount.h (100%)
rename scripts/{ => ftrace}/recordmcount.pl (100%)
mode change 100755 => 100644
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 01/11] ftrace: move recordmcount tools to scripts/ftrace
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, John F . Reiser, x86, linux-kernel, linux-mips,
Jessica Yu, sparclinux, linux-kbuild, Thomas Gleixner,
linuxppc-dev, linux-riscv, linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
Move ftrace tools to its own directory. We will add another tool later.
Cc: John F. Reiser <jreiser@BitWagon.com>
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
scripts/.gitignore | 1 -
scripts/Makefile | 2 +-
scripts/Makefile.build | 10 +++++-----
scripts/ftrace/.gitignore | 4 ++++
scripts/ftrace/Makefile | 4 ++++
scripts/{ => ftrace}/recordmcount.c | 0
scripts/{ => ftrace}/recordmcount.h | 0
scripts/{ => ftrace}/recordmcount.pl | 0
8 files changed, 14 insertions(+), 7 deletions(-)
create mode 100644 scripts/ftrace/.gitignore
create mode 100644 scripts/ftrace/Makefile
rename scripts/{ => ftrace}/recordmcount.c (100%)
rename scripts/{ => ftrace}/recordmcount.h (100%)
rename scripts/{ => ftrace}/recordmcount.pl (100%)
mode change 100755 => 100644
diff --git a/scripts/.gitignore b/scripts/.gitignore
index 17f8cef88fa8..1b5b5d595d80 100644
--- a/scripts/.gitignore
+++ b/scripts/.gitignore
@@ -6,7 +6,6 @@ conmakehash
kallsyms
pnmtologo
unifdef
-recordmcount
sortextable
asn1_compiler
extract-cert
diff --git a/scripts/Makefile b/scripts/Makefile
index 16bcb8087899..d5992def49a8 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -14,7 +14,6 @@ hostprogs-$(CONFIG_BUILD_BIN2C) += bin2c
hostprogs-$(CONFIG_KALLSYMS) += kallsyms
hostprogs-$(CONFIG_LOGO) += pnmtologo
hostprogs-$(CONFIG_VT) += conmakehash
-hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount
hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable
hostprogs-$(CONFIG_ASN1) += asn1_compiler
hostprogs-$(CONFIG_MODULE_SIG) += sign-file
@@ -34,6 +33,7 @@ hostprogs-y += unifdef
subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
subdir-$(CONFIG_MODVERSIONS) += genksyms
subdir-$(CONFIG_SECURITY_SELINUX) += selinux
+subdir-$(CONFIG_FTRACE) += ftrace
# Let clean descend into subdirs
subdir- += basic dtc gdb kconfig mod package
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 2f66ed388d1c..67558983c518 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -188,18 +188,18 @@ endif
# files, including recordmcount.
sub_cmd_record_mcount = \
if [ $(@) != "scripts/mod/empty.o" ]; then \
- $(objtree)/scripts/recordmcount $(RECORDMCOUNT_FLAGS) "$(@)"; \
+ $(objtree)/scripts/ftrace/recordmcount $(RECORDMCOUNT_FLAGS) "$(@)"; \
fi;
-recordmcount_source := $(srctree)/scripts/recordmcount.c \
- $(srctree)/scripts/recordmcount.h
+recordmcount_source := $(srctree)/scripts/ftrace/recordmcount.c \
+ $(srctree)/scripts/ftrace/recordmcount.h
else
-sub_cmd_record_mcount = perl $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
+sub_cmd_record_mcount = perl $(srctree)/scripts/ftrace/recordmcount.pl "$(ARCH)" \
"$(if $(CONFIG_CPU_BIG_ENDIAN),big,little)" \
"$(if $(CONFIG_64BIT),64,32)" \
"$(OBJDUMP)" "$(OBJCOPY)" "$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS)" \
"$(LD) $(KBUILD_LDFLAGS)" "$(NM)" "$(RM)" "$(MV)" \
"$(if $(part-of-module),1,0)" "$(@)";
-recordmcount_source := $(srctree)/scripts/recordmcount.pl
+recordmcount_source := $(srctree)/scripts/ftrace/recordmcount.pl
endif # BUILD_C_RECORDMCOUNT
cmd_record_mcount = $(if $(findstring $(strip $(CC_FLAGS_FTRACE)),$(_c_flags)), \
$(sub_cmd_record_mcount))
diff --git a/scripts/ftrace/.gitignore b/scripts/ftrace/.gitignore
new file mode 100644
index 000000000000..54d582c8faad
--- /dev/null
+++ b/scripts/ftrace/.gitignore
@@ -0,0 +1,4 @@
+#
+# Generated files
+#
+recordmcount
diff --git a/scripts/ftrace/Makefile b/scripts/ftrace/Makefile
new file mode 100644
index 000000000000..6797e51473e5
--- /dev/null
+++ b/scripts/ftrace/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+
+hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount
+always := $(hostprogs-y)
diff --git a/scripts/recordmcount.c b/scripts/ftrace/recordmcount.c
similarity index 100%
rename from scripts/recordmcount.c
rename to scripts/ftrace/recordmcount.c
diff --git a/scripts/recordmcount.h b/scripts/ftrace/recordmcount.h
similarity index 100%
rename from scripts/recordmcount.h
rename to scripts/ftrace/recordmcount.h
diff --git a/scripts/recordmcount.pl b/scripts/ftrace/recordmcount.pl
old mode 100755
new mode 100644
similarity index 100%
rename from scripts/recordmcount.pl
rename to scripts/ftrace/recordmcount.pl
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 02/11] ftrace: introduce new building tool funcprototype
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
This is a new ftrace tool to implement CONFIG_FTRACE_FUNC_PROTOTYPE
feature which allow ftrace record function parameters and return value
(see later patches).
Essentially funcprototype extracts only necessary information from
the DWARF debug sections in the ELF object file, including function
return type, parameter names and parameter data types. Then they will
be built into the original ELF object.
Here is an example for function memremap() in kernel/iomem.o. The
declaration is:
void *memremap(resource_size_t offset, size_t size, unsigned long flags)
The output of funcprototype tool is:
.section __funcprotostr, "a"
.P_memremap_0:
.string "offset"
.P_memremap_1:
.string "size"
.P_memremap_2:
.string "flags"
.section __funcproto, "a"
.quad memremap
.byte 0x8
.byte 0x3
.quad .P_memremap_0
.byte 0x8
.byte 0x55
.byte 0x0
.quad .P_memremap_1
.byte 0x8
.byte 0x54
.byte 0x0
.quad .P_memremap_2
.byte 0x8
.byte 0x51
.byte 0x0
The strings are placed in '__funcprotostr' section, and prototype
information is placed in '__funcproto' section. It equals to below
C struct:
struct func_param {
char *name;
uint8_t type;
uint8_t loc[2];
} __packed;
struct func_prototype {
unsigned long ip;
uint8_t ret_type;
uint8_t nr_param;
struct func_param params[0];
} __packed;
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
kernel/trace/Kconfig | 19 ++
scripts/Makefile.build | 18 +-
scripts/ftrace/.gitignore | 2 +
scripts/ftrace/Makefile | 7 +-
scripts/ftrace/funcprototype.c | 576 +++++++++++++++++++++++++++++++++
5 files changed, 620 insertions(+), 2 deletions(-)
create mode 100644 scripts/ftrace/funcprototype.c
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 98da8998c25c..20d1b0ae114d 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -38,6 +38,9 @@ config HAVE_FTRACE_MCOUNT_RECORD
help
See Documentation/trace/ftrace-design.rst
+config HAVE_FTRACE_FUNC_PROTOTYPE
+ bool
+
config HAVE_SYSCALL_TRACEPOINTS
bool
help
@@ -170,6 +173,22 @@ config FUNCTION_GRAPH_TRACER
the return value. This is done by setting the current return
address on the current task structure into a stack of calls.
+config FTRACE_FUNC_PROTOTYPE
+ bool "Support recording function parameters and return value"
+ default n
+ depends on DYNAMIC_FTRACE
+ depends on HAVE_FTRACE_FUNC_PROTOTYPE
+ depends on FUNCTION_GRAPH_TRACER
+ help
+ Enable the Function Graph Tracer to record function parameters and
+ return value. It can be dynamically enabled/disabled by the
+ 'record-funcproto' trace option.
+
+ By enabling this, function prototype information is built into
+ kernel. And the kernel size will increase by approximately 2%.
+
+ Say N if unsure.
+
config TRACE_PREEMPT_TOGGLE
bool
help
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 67558983c518..d56850808d96 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -206,6 +206,21 @@ cmd_record_mcount = $(if $(findstring $(strip $(CC_FLAGS_FTRACE)),$(_c_flags)),
endif # CC_USING_RECORD_MCOUNT
endif # CONFIG_FTRACE_MCOUNT_RECORD
+ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+sub_cmd_funcprototype = \
+ $(srctree)/scripts/ftrace/funcprototype "$(@)" | \
+ $(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) -c \
+ -o $(@D)/.tmp_$(@F:.o=.funcprototype) -x assembler -; \
+ $(LD) $(ld_flags) -r -o $(@D)/.tmp_$(@F) $@ $(@D)/.tmp_$(@F:.o=.funcprototype); \
+ mv -f $(@D)/.tmp_$(@F) $@; \
+ rm -f $(@D)/.tmp_$(@F:.o=.funcprototype);
+cmd_funcprototype = \
+ if $(OBJDUMP) -h $@ | grep -q __mcount_loc; then \
+ $(sub_cmd_funcprototype) \
+ fi
+funcprototype_source := $(srctree)/scripts/ftrace/funcprototype.c
+endif # CONFIG_FTRACE_FUNC_PROTOTYPE
+
ifdef CONFIG_STACK_VALIDATION
ifneq ($(SKIP_STACK_VALIDATION),1)
@@ -259,6 +274,7 @@ define rule_cc_o_c
$(call cmd,objtool)
$(call cmd,modversions_c)
$(call cmd,record_mcount)
+ $(call cmd,funcprototype)
endef
define rule_as_o_S
@@ -276,7 +292,7 @@ cmd_undef_syms = echo
endif
# Built-in and composite module parts
-$(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
+$(obj)/%.o: $(src)/%.c $(recordmcount_source) $(funcprototype_source) $(objtool_dep) FORCE
$(call cmd,force_checksrc)
$(call if_changed_rule,cc_o_c)
diff --git a/scripts/ftrace/.gitignore b/scripts/ftrace/.gitignore
index 54d582c8faad..92aa4f335656 100644
--- a/scripts/ftrace/.gitignore
+++ b/scripts/ftrace/.gitignore
@@ -2,3 +2,5 @@
# Generated files
#
recordmcount
+funcprototype
+
diff --git a/scripts/ftrace/Makefile b/scripts/ftrace/Makefile
index 6797e51473e5..c44d131b075c 100644
--- a/scripts/ftrace/Makefile
+++ b/scripts/ftrace/Makefile
@@ -1,4 +1,9 @@
# SPDX-License-Identifier: GPL-2.0
-hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount
+hostprogs-$(BUILD_C_RECORDMCOUNT) := recordmcount
+
+hostprogs-$(CONFIG_FTRACE_FUNC_PROTOTYPE) += funcprototype
+HOSTLDLIBS_funcprototype += -lelf
+HOSTLDLIBS_funcprototype += -ldw
+
always := $(hostprogs-y)
diff --git a/scripts/ftrace/funcprototype.c b/scripts/ftrace/funcprototype.c
new file mode 100644
index 000000000000..064724047b19
--- /dev/null
+++ b/scripts/ftrace/funcprototype.c
@@ -0,0 +1,576 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * funcprototype.c: generate function prototypes of the locations of calls to
+ * 'mcount' so that ftrace can record function parameters and return value.
+ *
+ * Copyright 2019 Changbin Du <changbin.du@gmail.com>. All rights reserved.
+ *
+ * Usage: funcprototype [OPTION...] elf-file
+ *
+ * Here is an example for function memremap() in kernel/iomem.o. The
+ * declaration is:
+ * void *memremap(resource_size_t offset, size_t size, unsigned long flags)
+ *
+ * The output of funcprototype tool is:
+ * .section __funcprotostr, "a"
+ * .P_memremap_0:
+ * .string "offset"
+ * .P_memremap_1:
+ * .string "size"
+ * .P_memremap_2:
+ * .string "flags"
+ *
+ * .section __funcproto, "a"
+ * .quad memremap
+ * .byte 0x8
+ * .byte 0x3
+ * .quad .P_memremap_0
+ * .byte 0x8
+ * .byte 0x55
+ * .byte 0x0
+ * .quad .P_memremap_1
+ * .byte 0x8
+ * .byte 0x54
+ * .byte 0x0
+ * .quad .P_memremap_2
+ * .byte 0x8
+ * .byte 0x51
+ * .byte 0x0
+ */
+
+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <err.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <argp.h>
+#include <libelf.h>
+#include <gelf.h>
+#include <dwarf.h>
+#include <elfutils/libdw.h>
+#include <elfutils/libdwfl.h>
+
+struct func_param {
+ char *name;
+ uint8_t type;
+ u_int8_t loc[2]; /* Location expression, loc[0] is opcode */
+};
+
+struct func_prototype {
+ struct func_prototype *next;
+ bool skip;
+
+ char *name;
+ uint8_t ret_type;
+ uint8_t nr_param;
+ struct func_param *params;
+};
+
+#define MK_TYPE(sign, size) (((!!sign) << 7) | size)
+
+static bool is_64bit_obj;
+static struct func_prototype *func_prototype_list;
+
+
+static struct func_prototype *func_prototype_list_add_new(const char *name)
+{
+ struct func_prototype *proto;
+
+ proto = malloc(sizeof(*proto));
+ if (!proto)
+ errx(1, "no memory");
+ memset(proto, 0, sizeof(*proto));
+
+ proto->name = strdup(name);
+ if (!proto->name)
+ errx(1, "no memory");
+
+ if (!func_prototype_list) {
+ proto->next = NULL;
+ func_prototype_list = proto;
+ } else {
+ proto->next = func_prototype_list->next;
+ func_prototype_list->next = proto;
+ }
+
+ return proto;
+}
+
+static struct func_prototype *func_prototype_list_search(const char *name)
+{
+ struct func_prototype *proto;
+
+ for (proto = func_prototype_list; proto != NULL; proto = proto->next) {
+ if (!strcmp(proto->name, name))
+ return proto;
+ };
+ return NULL;
+}
+
+static void func_prototype_list_dumpnames(void)
+{
+ struct func_prototype *proto;
+
+ for (proto = func_prototype_list; proto != NULL; proto = proto->next)
+ printf("%s\n", proto->name);
+}
+
+static void func_prototype_list_destroy(void)
+{
+ struct func_prototype *proto;
+ int i;
+
+ while (func_prototype_list) {
+ proto = func_prototype_list;
+ func_prototype_list = func_prototype_list->next;
+
+ free(proto->name);
+ if (proto->params) {
+ for (i = 0; i < proto->nr_param; i++)
+ free(proto->params[i].name);
+ free(proto->params);
+ }
+ free(proto);
+ }
+}
+
+static bool is_mcount(char *name)
+{
+ return !strcmp(name, "__fentry__") ||
+ !strcmp(name, "_mcount") ||
+ !strcmp(name, "mcount");
+}
+
+static void check_elf(Elf *elf)
+{
+ GElf_Ehdr ehdr_mem;
+ GElf_Ehdr *ehdr;
+
+ ehdr = gelf_getehdr(elf, &ehdr_mem);
+ if (!ehdr)
+ errx(1, "cannot read ELF header");
+
+ is_64bit_obj = gelf_getclass(elf) == ELFCLASS64;
+
+ switch (ehdr->e_machine) {
+ case EM_386:
+ case EM_X86_64:
+ break;
+ default:
+ errx(1, "unsupported arch %d", ehdr->e_machine);
+ }
+}
+
+/**
+ * Search the symbole table to get the entry which matches @scn and @offset
+ * from relocation talbe.
+ */
+static char *search_mcount_caller(Elf *elf, GElf_Shdr *symshdr,
+ Elf_Data *symdata, int scn, int offset)
+{
+ int ndx;
+ char *caller;
+
+ for (ndx = 0; ndx < symshdr->sh_size / symshdr->sh_entsize; ++ndx) {
+ GElf_Sym sym;
+
+ gelf_getsym(symdata, ndx, &sym);
+
+ /* TODO: add local symobl support. */
+ if (GELF_ST_BIND(sym.st_info) == STB_GLOBAL &&
+ scn == sym.st_shndx && (offset >= sym.st_value) &&
+ (offset < sym.st_value + sym.st_size)) {
+ caller = elf_strptr(elf, symshdr->sh_link, sym.st_name);
+ return caller;
+ }
+ }
+
+ return NULL;
+}
+
+/* Get all functions that call to mcount. */
+static void get_mcount_callers(const char *elf_file)
+{
+ Elf *elf;
+ Elf_Scn *scn = NULL;
+ GElf_Shdr shdr;
+ int fd;
+ int ndx;
+
+ fd = open(elf_file, O_RDONLY);
+ if (fd < 0)
+ errx(1, "can not open %s", elf_file);
+
+ elf_version(EV_CURRENT);
+ elf = elf_begin(fd, ELF_C_READ, NULL);
+
+ check_elf(elf);
+
+ while ((scn = elf_nextscn(elf, scn)) != NULL) {
+ gelf_getshdr(scn, &shdr);
+
+ if (shdr.sh_type == SHT_REL || shdr.sh_type == SHT_RELA) {
+ Elf_Data *data = elf_getdata(scn, NULL);
+ Elf_Scn *symscn = elf_getscn(elf, shdr.sh_link);
+ Elf_Data *symdata = elf_getdata(symscn, NULL);
+ GElf_Shdr symshdr_mem;
+ GElf_Shdr *symshdr = gelf_getshdr(symscn, &symshdr_mem);
+
+ for (ndx = 0; ndx < shdr.sh_size / shdr.sh_entsize;
+ ++ndx) {
+ unsigned long sym_index;
+ unsigned long offset;
+ GElf_Sym sym;
+ char *symname;
+
+ if (shdr.sh_type == SHT_REL) {
+ GElf_Rel rel_mem;
+ GElf_Rel *rel = gelf_getrel(data, ndx,
+ &rel_mem);
+ sym_index = GELF_R_SYM(rel->r_info);
+ offset = rel->r_offset;
+ } else {
+ GElf_Rela rela_mem;
+ GElf_Rela *rela = gelf_getrela(
+ data, ndx, &rela_mem);
+ sym_index = GELF_R_SYM(rela->r_info);
+ offset = rela->r_offset;
+ }
+
+ gelf_getsym(symdata, sym_index, &sym);
+ symname = elf_strptr(elf, symshdr->sh_link,
+ sym.st_name);
+
+ if (is_mcount(symname)) {
+ const char *caller;
+
+ caller = search_mcount_caller(
+ elf, symshdr, symdata,
+ shdr.sh_info, offset);
+ if (caller)
+ func_prototype_list_add_new(caller);
+ }
+ }
+ }
+ }
+
+ elf_end(elf);
+ close(fd);
+}
+
+/*
+ * Get a variable size and sign info.
+ * TODO: Determine the expected display format. (e.g. size_t for "%lu").
+ */
+static void die_type_sign_bytes(Dwarf_Die *die, bool *is_signed, int *bytes)
+{
+ Dwarf_Attribute attr;
+ Dwarf_Die type;
+ int ret;
+
+ *bytes = 0;
+ *is_signed = false;
+
+ ret = dwarf_peel_type(dwarf_formref_die(
+ dwarf_attr_integrate(die, DW_AT_type, &attr), &type),
+ &type);
+ if (ret == 0) {
+ Dwarf_Word val;
+
+ ret = dwarf_formudata(dwarf_attr(&type, DW_AT_encoding,
+ &attr), &val);
+ if (ret == 0)
+ *is_signed = (val == DW_ATE_signed) ||
+ (val == DW_ATE_signed_char);
+
+ if (dwarf_aggregate_size(&type, &val) == 0)
+ *bytes = val;
+ }
+}
+
+static int get_func_nr_params(Dwarf_Die *funcdie)
+{
+ Dwarf_Die child;
+ int count = 0;
+
+ if (dwarf_child(funcdie, &child) == 0) {
+ do {
+ if (dwarf_tag(&child) == DW_TAG_formal_parameter)
+ count++;
+ } while (dwarf_siblingof(&child, &child) == 0);
+ }
+
+ return count;
+}
+
+static int get_loc_expr(const char *fname, Dwarf_Op *loc, uint8_t expr[2])
+{
+ int ret = 0;
+ int off;
+
+ switch (loc->atom) {
+ case DW_OP_reg0 ... DW_OP_reg31:
+ expr[0] = loc->atom;
+ expr[1] = 0;
+ break;
+ case DW_OP_fbreg:
+ off = (int32_t)loc->number;
+
+ /*
+ * Very few functions have number that exceeds
+ * (SCHAR_MIN, SCHAR_MAX). We skip these
+ * functions to keep protrotype data as small
+ * as possilbe.
+ */
+ if (off > SCHAR_MAX || off < SCHAR_MIN) {
+ warnx("%s: loc fbreg offset %d too large", fname, off);
+ ret = -1;
+ } else {
+ expr[0] = loc->atom;
+ expr[1] = off; /* The operand is signed */
+ }
+ break;
+ case DW_OP_breg0 ... DW_OP_breg31:
+ off = (int32_t)loc->number;
+
+ if (off > SCHAR_MAX || off < SCHAR_MIN) {
+ warnx("%s: loc bregx offset %d too large", fname, off);
+ ret = -1;
+ } else {
+ expr[0] = loc->atom;
+ expr[1] = off;
+ }
+ break;
+ default:
+ warnx("%s: unsupported loc operation 0x%x",
+ fname, loc->atom);
+ ret = -1;
+ };
+
+ return ret;
+}
+
+static int handle_function(Dwarf_Die *funcdie, void *arg)
+{
+ const char *name = dwarf_diename(funcdie);
+ Dwarf_Addr func_addr;
+ Dwarf_Die child;
+ struct func_prototype *proto;
+ int nr_params;
+ int sz, n = 0;
+
+ if (!dwarf_hasattr(funcdie, DW_AT_low_pc))
+ return 0;
+
+ /* Such symbol is a local function generated by GCC ipa-fnsplit. */
+ if (!dwarf_hasattr(funcdie, DW_AT_name))
+ return 0;
+
+ /* check whether it is a mcount caller. */
+ proto = func_prototype_list_search(name);
+ if (!proto)
+ return 0;
+
+ nr_params = get_func_nr_params(funcdie);
+ sz = sizeof(proto->params[0]) * nr_params;
+ proto->params = malloc(sz);
+ if (!proto->params)
+ errx(1, "no memory");
+
+ memset(proto->params, 0, sz);
+
+ dwarf_lowpc(funcdie, &func_addr);
+
+ /* get function return type */
+ if (dwarf_hasattr(funcdie, DW_AT_type)) {
+ bool is_signed;
+ int bytes;
+
+ die_type_sign_bytes(funcdie, &is_signed, &bytes);
+ proto->ret_type = MK_TYPE(is_signed, bytes);
+ } else
+ proto->ret_type = 0;
+
+ /* process function parameters. */
+ if (dwarf_child(funcdie, &child) == 0) {
+ do {
+ if (dwarf_tag(&child) == DW_TAG_formal_parameter) {
+ Dwarf_Attribute locattr;
+ Dwarf_Op *loc;
+ size_t nloc = 0;
+ bool is_signed;
+ int bytes;
+
+ die_type_sign_bytes(&child, &is_signed, &bytes);
+ proto->params[n].name = strdup(dwarf_diename(&child));
+ proto->params[n].type = MK_TYPE(is_signed, bytes);
+
+ if (!dwarf_hasattr(&child, DW_AT_location))
+ errx(1, "%s: no location attr", name);
+
+ dwarf_attr(&child, DW_AT_location, &locattr);
+ if (dwarf_getlocation(&locattr, &loc, &nloc) < 0) {
+ Dwarf_Addr base, begin, end;
+
+ if (dwarf_getlocations(
+ &locattr, 0, &base,
+ &begin, &end, &loc,
+ &nloc) <= 0)
+ errx(1, "%s: no param loc info",
+ name);
+ }
+ if (get_loc_expr(name, loc, proto->params[n].loc)) {
+ /* skip this function. */
+ proto->skip = true;
+ return 0;
+ }
+
+ n++;
+ };
+ } while (dwarf_siblingof(&child, &child) == 0);
+ }
+
+ proto->nr_param = n;
+ return 0;
+}
+
+static const Dwfl_Callbacks offline_callbacks = {
+ .find_debuginfo = dwfl_standard_find_debuginfo,
+ .section_address = dwfl_offline_section_address,
+};
+
+/* Iterate each DW_TAG_subprogram DIE to get their prototype info. */
+static void dwarf_get_prototypes(const char *elf_file)
+{
+ Dwfl *dwfl = NULL;
+ Dwarf_Die *cu = NULL;
+ Dwarf_Addr dwbias;
+ int ret;
+
+ dwfl = dwfl_begin(&offline_callbacks);
+ if (dwfl == NULL)
+ errx(1, "dwfl fail");
+
+ if (dwfl_report_offline(dwfl, "", elf_file, -1) == NULL)
+ errx(1, "dwfl report fail");
+
+ ret = dwfl_report_end(dwfl, NULL, NULL);
+ assert(ret == 0);
+
+ while ((cu = dwfl_nextcu(dwfl, cu, &dwbias)) != NULL)
+ dwarf_getfuncs(cu, &handle_function, NULL, 0);
+}
+
+static void print_prototypes_assembly(void)
+{
+ struct func_prototype *proto;
+ int i;
+
+ if (!func_prototype_list)
+ return;
+
+ printf(" .section __funcprotostr, \"a\"\n");
+ for (proto = func_prototype_list; proto != NULL; proto = proto->next) {
+ if (proto->skip)
+ continue;
+ for (i = 0; i < proto->nr_param; i++) {
+ printf(".P_%s_%d:\n", proto->name, i);
+ printf(" .string \"%s\"\n", proto->params[i].name);
+ }
+ };
+
+ printf("\n .section __funcproto, \"a\"\n");
+ for (proto = func_prototype_list; proto != NULL; proto = proto->next) {
+ if (proto->skip)
+ continue;
+ if (is_64bit_obj)
+ printf(" .quad %s\n", proto->name);
+ else
+ printf(" .long %s\n", proto->name);
+ printf(" .byte 0x%x\n", proto->ret_type);
+ printf(" .byte 0x%x\n", proto->nr_param);
+ for (i = 0; i < proto->nr_param; i++) {
+ if (is_64bit_obj)
+ printf(" .quad .P_%s_%d\n", proto->name, i);
+ else
+ printf(" .long .P_%s_%d\n", proto->name, i);
+ printf(" .byte 0x%x\n", proto->params[i].type);
+ printf(" .byte 0x%x\n", proto->params[i].loc[0]);
+ printf(" .byte 0x%x\n", proto->params[i].loc[1]);
+ }
+ printf("\n");
+ };
+}
+
+/* Program documentation. */
+static char doc[] =
+ "funcprototype -- a program to generate mcount caller prototypes";
+
+/* A description of the arguments we accept. */
+static const char args_doc[] = "elf-file";
+
+/* The options we understand. */
+static struct argp_option options[] = { { "mcount-callers", 'm', 0, 0,
+ "show mcount callers only" },
+ { 0 } };
+
+struct arguments {
+ char *elf_file;
+ int show_callers_only;
+};
+
+/* Parse options. */
+static error_t parse_opt(int key, char *arg, struct argp_state *state)
+{
+ struct arguments *arguments = state->input;
+
+ switch (key) {
+ case 'm':
+ arguments->show_callers_only = 1;
+ break;
+ case ARGP_KEY_ARG:
+ if (state->arg_num > 2) {
+ /* Too many arguments. */
+ argp_usage(state);
+ }
+ arguments->elf_file = arg;
+ break;
+ case ARGP_KEY_END:
+ if (state->arg_num < 1)
+ /* Not enough arguments. */
+ argp_usage(state);
+ break;
+ default:
+ return ARGP_ERR_UNKNOWN;
+ }
+ return 0;
+}
+
+/* Our argp parser. */
+static struct argp argp = { options, parse_opt, args_doc, doc };
+
+int main(int argc, char *argv[])
+{
+ struct arguments arguments;
+
+ arguments.show_callers_only = 0;
+ argp_parse(&argp, argc, argv, 0, 0, &arguments);
+
+ get_mcount_callers(arguments.elf_file);
+
+ if (arguments.show_callers_only) {
+ func_prototype_list_dumpnames();
+ goto free;
+ }
+
+ dwarf_get_prototypes(arguments.elf_file);
+ print_prototypes_assembly();
+
+free:
+ func_prototype_list_destroy();
+ return 0;
+}
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 03/11] asm-generic: add generic dwarf definition
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
Add generic DWARF constant definitions. We will use it later.
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
include/asm-generic/dwarf.h | 199 ++++++++++++++++++++++++++++++++++++
1 file changed, 199 insertions(+)
create mode 100644 include/asm-generic/dwarf.h
diff --git a/include/asm-generic/dwarf.h b/include/asm-generic/dwarf.h
new file mode 100644
index 000000000000..c705633c2a8f
--- /dev/null
+++ b/include/asm-generic/dwarf.h
@@ -0,0 +1,199 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Architecture independent definitions of DWARF.
+ *
+ * Copyright (C) 2019 Changbin Du <changbin.du@gmail.com>
+ */
+#ifndef __ASM_GENERIC_DWARF_H
+#define __ASM_GENERIC_DWARF_H
+
+/*
+ * DWARF expression operations
+ */
+#define DW_OP_addr 0x03
+#define DW_OP_deref 0x06
+#define DW_OP_const1u 0x08
+#define DW_OP_const1s 0x09
+#define DW_OP_const2u 0x0a
+#define DW_OP_const2s 0x0b
+#define DW_OP_const4u 0x0c
+#define DW_OP_const4s 0x0d
+#define DW_OP_const8u 0x0e
+#define DW_OP_const8s 0x0f
+#define DW_OP_constu 0x10
+#define DW_OP_consts 0x11
+#define DW_OP_dup 0x12
+#define DW_OP_drop 0x13
+#define DW_OP_over 0x14
+#define DW_OP_pick 0x15
+#define DW_OP_swap 0x16
+#define DW_OP_rot 0x17
+#define DW_OP_xderef 0x18
+#define DW_OP_abs 0x19
+#define DW_OP_and 0x1a
+#define DW_OP_div 0x1b
+#define DW_OP_minus 0x1c
+#define DW_OP_mod 0x1d
+#define DW_OP_mul 0x1e
+#define DW_OP_neg 0x1f
+#define DW_OP_not 0x20
+#define DW_OP_or 0x21
+#define DW_OP_plus 0x22
+#define DW_OP_plus_uconst 0x23
+#define DW_OP_shl 0x24
+#define DW_OP_shr 0x25
+#define DW_OP_shra 0x26
+#define DW_OP_xor 0x27
+#define DW_OP_skip 0x2f
+#define DW_OP_bra 0x28
+#define DW_OP_eq 0x29
+#define DW_OP_ge 0x2a
+#define DW_OP_gt 0x2b
+#define DW_OP_le 0x2c
+#define DW_OP_lt 0x2d
+#define DW_OP_ne 0x2e
+#define DW_OP_lit0 0x30
+#define DW_OP_lit1 0x31
+#define DW_OP_lit2 0x32
+#define DW_OP_lit3 0x33
+#define DW_OP_lit4 0x34
+#define DW_OP_lit5 0x35
+#define DW_OP_lit6 0x36
+#define DW_OP_lit7 0x37
+#define DW_OP_lit8 0x38
+#define DW_OP_lit9 0x39
+#define DW_OP_lit10 0x3a
+#define DW_OP_lit11 0x3b
+#define DW_OP_lit12 0x3c
+#define DW_OP_lit13 0x3d
+#define DW_OP_lit14 0x3e
+#define DW_OP_lit15 0x3f
+#define DW_OP_lit16 0x40
+#define DW_OP_lit17 0x41
+#define DW_OP_lit18 0x42
+#define DW_OP_lit19 0x43
+#define DW_OP_lit20 0x44
+#define DW_OP_lit21 0x45
+#define DW_OP_lit22 0x46
+#define DW_OP_lit23 0x47
+#define DW_OP_lit24 0x48
+#define DW_OP_lit25 0x49
+#define DW_OP_lit26 0x4a
+#define DW_OP_lit27 0x4b
+#define DW_OP_lit28 0x4c
+#define DW_OP_lit29 0x4d
+#define DW_OP_lit30 0x4e
+#define DW_OP_lit31 0x4f
+#define DW_OP_reg0 0x50
+#define DW_OP_reg1 0x51
+#define DW_OP_reg2 0x52
+#define DW_OP_reg3 0x53
+#define DW_OP_reg4 0x54
+#define DW_OP_reg5 0x55
+#define DW_OP_reg6 0x56
+#define DW_OP_reg7 0x57
+#define DW_OP_reg8 0x58
+#define DW_OP_reg9 0x59
+#define DW_OP_reg10 0x5a
+#define DW_OP_reg11 0x5b
+#define DW_OP_reg12 0x5c
+#define DW_OP_reg13 0x5d
+#define DW_OP_reg14 0x5e
+#define DW_OP_reg15 0x5f
+#define DW_OP_reg16 0x60
+#define DW_OP_reg17 0x61
+#define DW_OP_reg18 0x62
+#define DW_OP_reg19 0x63
+#define DW_OP_reg20 0x64
+#define DW_OP_reg21 0x65
+#define DW_OP_reg22 0x66
+#define DW_OP_reg23 0x67
+#define DW_OP_reg24 0x68
+#define DW_OP_reg25 0x69
+#define DW_OP_reg26 0x6a
+#define DW_OP_reg27 0x6b
+#define DW_OP_reg28 0x6c
+#define DW_OP_reg29 0x6d
+#define DW_OP_reg30 0x6e
+#define DW_OP_reg31 0x6f
+#define DW_OP_breg0 0x70
+#define DW_OP_breg1 0x71
+#define DW_OP_breg2 0x72
+#define DW_OP_breg3 0x73
+#define DW_OP_breg4 0x74
+#define DW_OP_breg5 0x75
+#define DW_OP_breg6 0x76
+#define DW_OP_breg7 0x77
+#define DW_OP_breg8 0x78
+#define DW_OP_breg9 0x79
+#define DW_OP_breg10 0x7a
+#define DW_OP_breg11 0x7b
+#define DW_OP_breg12 0x7c
+#define DW_OP_breg13 0x7d
+#define DW_OP_breg14 0x7e
+#define DW_OP_breg15 0x7f
+#define DW_OP_breg16 0x80
+#define DW_OP_breg17 0x81
+#define DW_OP_breg18 0x82
+#define DW_OP_breg19 0x83
+#define DW_OP_breg20 0x84
+#define DW_OP_breg21 0x85
+#define DW_OP_breg22 0x86
+#define DW_OP_breg23 0x87
+#define DW_OP_breg24 0x88
+#define DW_OP_breg25 0x89
+#define DW_OP_breg26 0x8a
+#define DW_OP_breg27 0x8b
+#define DW_OP_breg28 0x8c
+#define DW_OP_breg29 0x8d
+#define DW_OP_breg30 0x8e
+#define DW_OP_breg31 0x8f
+#define DW_OP_regx 0x90
+#define DW_OP_fbreg 0x91
+#define DW_OP_bregx 0x92
+#define DW_OP_piece 0x93
+#define DW_OP_deref_size 0x94
+#define DW_OP_xderef_size 0x95
+#define DW_OP_nop 0x96
+#define DW_OP_push_object_address 0x97
+#define DW_OP_call2 0x98
+#define DW_OP_call4 0x99
+#define DW_OP_call_ref 0x9a
+#define DW_OP_form_tls_address 0x9b
+#define DW_OP_call_frame_cfa 0x9c
+#define DW_OP_bit_piece 0x9d
+#define DW_OP_implicit_value 0x9e
+#define DW_OP_stack_value 0x9f
+#define DW_OP_implicit_pointer 0xa0
+#define DW_OP_addrx 0xa1
+#define DW_OP_constx 0xa2
+#define DW_OP_entry_value 0xa3
+#define DW_OP_const_type 0xa4
+#define DW_OP_regval_type 0xa5
+#define DW_OP_deref_type 0xa6
+#define DW_OP_xderef_type 0xa7
+#define DW_OP_convert 0xa8
+#define DW_OP_reinterpret 0xa9
+
+/* GNU extensions. */
+#define DW_OP_GNU_push_tls_address 0xe0
+#define DW_OP_GNU_uninit 0xf0
+#define DW_OP_GNU_encoded_addr 0xf1
+#define DW_OP_GNU_implicit_pointer 0xf2
+#define DW_OP_GNU_entry_value 0xf3
+#define DW_OP_GNU_const_type 0xf4
+#define DW_OP_GNU_regval_type 0xf5
+#define DW_OP_GNU_deref_type 0xf6
+#define DW_OP_GNU_convert 0xf7
+#define DW_OP_GNU_reinterpret 0xf9
+#define DW_OP_GNU_parameter_ref 0xfa
+
+/* GNU Debug Fission extensions. */
+#define DW_OP_GNU_addr_index 0xfb,
+#define DW_OP_GNU_const_index 0xfc
+#define DW_OP_GNU_variable_value 0xfd
+
+#define DW_OP_lo_user 0xe0
+#define DW_OP_hi_user 0xff
+
+#endif /* __ASM_GENERIC_DWARF_H */
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 04/11] ftrace/hash: add private data field
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
We will reuse ftrace_hash to lookup function prototype information. So
we need an additional field to bind ftrace_func_entry to prototype
information.
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
kernel/trace/ftrace.c | 17 +++++++----------
kernel/trace/trace.h | 6 ++++++
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index eca34503f178..a314f0768b2c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1017,11 +1017,6 @@ static bool update_all_ops;
# error Dynamic ftrace depends on MCOUNT_RECORD
#endif
-struct ftrace_func_entry {
- struct hlist_node hlist;
- unsigned long ip;
-};
-
struct ftrace_func_probe {
struct ftrace_probe_ops *probe_ops;
struct ftrace_ops ops;
@@ -1169,7 +1164,8 @@ static void __add_hash_entry(struct ftrace_hash *hash,
hash->count++;
}
-static int add_hash_entry(struct ftrace_hash *hash, unsigned long ip)
+static int add_hash_entry(struct ftrace_hash *hash, unsigned long ip,
+ void *priv)
{
struct ftrace_func_entry *entry;
@@ -1178,6 +1174,7 @@ static int add_hash_entry(struct ftrace_hash *hash, unsigned long ip)
return -ENOMEM;
entry->ip = ip;
+ entry->priv = priv;
__add_hash_entry(hash, entry);
return 0;
@@ -1346,7 +1343,7 @@ alloc_and_copy_ftrace_hash(int size_bits, struct ftrace_hash *hash)
size = 1 << hash->size_bits;
for (i = 0; i < size; i++) {
hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
- ret = add_hash_entry(new_hash, entry->ip);
+ ret = add_hash_entry(new_hash, entry->ip, NULL);
if (ret < 0)
goto free_hash;
}
@@ -3694,7 +3691,7 @@ enter_record(struct ftrace_hash *hash, struct dyn_ftrace *rec, int clear_filter)
if (entry)
return 0;
- ret = add_hash_entry(hash, rec->ip);
+ ret = add_hash_entry(hash, rec->ip, NULL);
}
return ret;
}
@@ -4700,7 +4697,7 @@ ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove)
return 0;
}
- return add_hash_entry(hash, ip);
+ return add_hash_entry(hash, ip, NULL);
}
static int
@@ -5380,7 +5377,7 @@ ftrace_graph_set_hash(struct ftrace_hash *hash, char *buffer)
if (entry)
continue;
- if (add_hash_entry(hash, rec->ip) < 0)
+ if (add_hash_entry(hash, rec->ip, NULL) < 0)
goto out;
} else {
if (entry) {
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 005f08629b8b..ad619c73a505 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -882,6 +882,12 @@ struct ftrace_hash {
struct rcu_head rcu;
};
+struct ftrace_func_entry {
+ struct hlist_node hlist;
+ unsigned long ip;
+ void *priv;
+};
+
struct ftrace_func_entry *
ftrace_lookup_ip(struct ftrace_hash *hash, unsigned long ip);
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 05/11] ftrace: create memcache for hash entries
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
When CONFIG_FTRACE_FUNC_PROTOTYPE is enabled, thousands of
ftrace_func_entry instances are created. So create a dedicated
memcache to enhance performance.
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
kernel/trace/ftrace.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a314f0768b2c..cfcb8dad93ea 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -94,6 +94,8 @@ struct ftrace_ops *function_trace_op __read_mostly = &ftrace_list_end;
/* What to set function_trace_op to */
static struct ftrace_ops *set_function_trace_op;
+struct kmem_cache *hash_entry_cache;
+
static bool ftrace_pids_enabled(struct ftrace_ops *ops)
{
struct trace_array *tr;
@@ -1169,7 +1171,7 @@ static int add_hash_entry(struct ftrace_hash *hash, unsigned long ip,
{
struct ftrace_func_entry *entry;
- entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+ entry = kmem_cache_alloc(hash_entry_cache, GFP_KERNEL);
if (!entry)
return -ENOMEM;
@@ -6153,6 +6155,15 @@ void __init ftrace_init(void)
if (ret)
goto failed;
+ hash_entry_cache = kmem_cache_create("ftrace-hash",
+ sizeof(struct ftrace_func_entry),
+ sizeof(struct ftrace_func_entry),
+ 0, NULL);
+ if (!hash_entry_cache) {
+ pr_err("failed to create ftrace hash entry cache\n");
+ goto failed;
+ }
+
count = __stop_mcount_loc - __start_mcount_loc;
if (!count) {
pr_info("ftrace: No functions to be traced?\n");
@@ -6172,6 +6183,10 @@ void __init ftrace_init(void)
return;
failed:
+ if (hash_entry_cache) {
+ kmem_cache_destroy(hash_entry_cache);
+ hash_entry_cache = NULL;
+ }
ftrace_disabled = 1;
}
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 06/11] ftrace: process function prototype data in vmlinux and modules
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
Walk through the '__funcproto' section in vmlinux and kernel modules.
For each item we add it to a new ftrace hash table ftrace_prototype_hash.
When unloading a module, its items are removed from hash table.
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
include/asm-generic/vmlinux.lds.h | 18 ++++++++
include/linux/ftrace.h | 18 ++++++++
include/linux/module.h | 4 ++
kernel/module.c | 25 ++++++++--
kernel/trace/ftrace.c | 76 ++++++++++++++++++++++++++++++-
kernel/trace/trace.h | 4 ++
6 files changed, 140 insertions(+), 5 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index cd28f63bfbc7..3b0a10cbf0ca 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -125,6 +125,23 @@
#define MCOUNT_REC()
#endif
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+#define FUNC_PROTOTYPE \
+ . = ALIGN(8); \
+ __funcprotostr : AT(ADDR(__funcprotostr) - LOAD_OFFSET) { \
+ KEEP(*(__funcprotostr)) \
+ } \
+ \
+ . = ALIGN(8); \
+ __funcproto : AT(ADDR(__funcproto) - LOAD_OFFSET) { \
+ __start_funcproto = .; \
+ KEEP(*(__funcproto)) \
+ __stop_funcproto = .; \
+ }
+#else
+#define FUNC_PROTOTYPE
+#endif
+
#ifdef CONFIG_TRACE_BRANCH_PROFILING
#define LIKELY_PROFILE() __start_annotated_branch_profile = .; \
KEEP(*(_ftrace_annotated_branch)) \
@@ -396,6 +413,7 @@
} \
\
TRACEDATA \
+ FUNC_PROTOTYPE \
\
/* Kernel symbol table: Normal symbols */ \
__ksymtab : AT(ADDR(__ksymtab) - LOAD_OFFSET) { \
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 8a8cb3c401b2..f5aab37a8c34 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -361,6 +361,24 @@ struct dyn_ftrace {
struct dyn_arch_ftrace arch;
};
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+struct func_param {
+ char *name;
+ uint8_t type;
+ uint8_t loc[2];
+} __packed;
+
+struct func_prototype {
+ unsigned long ip;
+ uint8_t ret_type;
+ uint8_t nr_param;
+ struct func_param params[0];
+} __packed;
+
+#define FTRACE_PROTOTYPE_SIGNED(t) (t & BIT(7))
+#define FTRACE_PROTOTYPE_SIZE(t) (t & GENMASK(6, 0))
+#endif
+
int ftrace_force_update(void);
int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip,
int remove, int reset);
diff --git a/include/linux/module.h b/include/linux/module.h
index 1455812dd325..516062dfe567 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -477,6 +477,10 @@ struct module {
unsigned int num_ftrace_callsites;
unsigned long *ftrace_callsites;
#endif
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ struct func_prototype *funcproto_start;
+ size_t funcproto_sec_size;
+#endif
#ifdef CONFIG_LIVEPATCH
bool klp; /* Is this a livepatch module? */
diff --git a/kernel/module.c b/kernel/module.c
index 9ee93421269c..1c5eea7b6a28 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -360,17 +360,30 @@ static void *section_addr(const struct load_info *info, const char *name)
return (void *)info->sechdrs[find_sec(info, name)].sh_addr;
}
+/* Get info of a module section. */
+static void *section_info(const struct load_info *info,
+ const char *name,
+ size_t *size)
+{
+ unsigned int sec = find_sec(info, name);
+
+ /* Section 0 has sh_addr 0 and sh_size 0. */
+ *size = info->sechdrs[sec].sh_size;
+ return (void *)info->sechdrs[sec].sh_addr;
+}
+
/* Find a module section, or NULL. Fill in number of "objects" in section. */
static void *section_objs(const struct load_info *info,
const char *name,
size_t object_size,
unsigned int *num)
{
- unsigned int sec = find_sec(info, name);
+ void *addr;
+ size_t sz;
- /* Section 0 has sh_addr 0 and sh_size 0. */
- *num = info->sechdrs[sec].sh_size / object_size;
- return (void *)info->sechdrs[sec].sh_addr;
+ addr = section_info(info, name, &sz);
+ *num = sz / object_size;
+ return addr;
}
/* Provided by the linker */
@@ -3140,6 +3153,10 @@ static int find_module_sections(struct module *mod, struct load_info *info)
sizeof(*mod->ftrace_callsites),
&mod->num_ftrace_callsites);
#endif
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ mod->funcproto_start = section_info(info, "__funcproto",
+ &mod->funcproto_sec_size);
+#endif
#ifdef CONFIG_FUNCTION_ERROR_INJECTION
mod->ei_funcs = section_objs(info, "_error_injection_whitelist",
sizeof(*mod->ei_funcs),
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index cfcb8dad93ea..438b8b47198f 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5060,6 +5060,9 @@ static DEFINE_MUTEX(graph_lock);
struct ftrace_hash *ftrace_graph_hash = EMPTY_HASH;
struct ftrace_hash *ftrace_graph_notrace_hash = EMPTY_HASH;
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+struct ftrace_hash *ftrace_prototype_hash = EMPTY_HASH;
+#endif
enum graph_filter_type {
GRAPH_FILTER_NOTRACE = 0,
@@ -5615,6 +5618,46 @@ static int ftrace_process_locs(struct module *mod,
return ret;
}
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+static int ftrace_process_funcproto(struct module *mod,
+ struct func_prototype *start,
+ struct func_prototype *end,
+ bool remove)
+{
+ struct ftrace_func_entry *ent;
+ struct func_prototype *proto;
+ int ret = 0;
+
+ mutex_lock(&ftrace_lock);
+
+restart:
+ proto = start;
+ while (proto < end) {
+ if (remove) {
+ ent = ftrace_lookup_ip(ftrace_prototype_hash,
+ proto->ip);
+ if (ent)
+ free_hash_entry(ftrace_prototype_hash, ent);
+ } else {
+ ret = add_hash_entry(ftrace_prototype_hash,
+ proto->ip, proto);
+ if (ret < 0) {
+ end = proto;
+ remove = 1;
+ goto restart;
+ }
+ }
+ proto = (struct func_prototype *)((char *)proto +
+ sizeof(*proto) +
+ sizeof(proto->params[0]) * proto->nr_param);
+ }
+
+ mutex_unlock(&ftrace_lock);
+
+ return ret;
+}
+#endif
+
struct ftrace_mod_func {
struct list_head list;
char *name;
@@ -5707,7 +5750,7 @@ static void ftrace_free_mod_map(struct rcu_head *rcu)
kfree(mod_map);
}
-void ftrace_release_mod(struct module *mod)
+void ftrace_release_dyn(struct module *mod)
{
struct ftrace_mod_map *mod_map;
struct ftrace_mod_map *n;
@@ -5773,6 +5816,17 @@ void ftrace_release_mod(struct module *mod)
}
}
+void ftrace_release_mod(struct module *mod)
+{
+ ftrace_release_dyn(mod);
+
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ ftrace_process_funcproto(mod, mod->funcproto_start,
+ (void *)mod->funcproto_start + mod->funcproto_sec_size,
+ true);
+#endif
+}
+
void ftrace_module_enable(struct module *mod)
{
struct dyn_ftrace *rec;
@@ -5852,6 +5906,11 @@ void ftrace_module_init(struct module *mod)
ftrace_process_locs(mod, mod->ftrace_callsites,
mod->ftrace_callsites + mod->num_ftrace_callsites);
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ ftrace_process_funcproto(mod, mod->funcproto_start,
+ (void *)mod->funcproto_start + mod->funcproto_sec_size,
+ false);
+#endif
}
static void save_ftrace_mod_rec(struct ftrace_mod_map *mod_map,
@@ -6146,6 +6205,10 @@ void __init ftrace_init(void)
{
extern unsigned long __start_mcount_loc[];
extern unsigned long __stop_mcount_loc[];
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ extern struct func_prototype __start_funcproto[];
+ extern struct func_prototype __stop_funcproto[];
+#endif
unsigned long count, flags;
int ret;
@@ -6179,6 +6242,17 @@ void __init ftrace_init(void)
__start_mcount_loc,
__stop_mcount_loc);
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ ftrace_prototype_hash = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
+ if (WARN_ON(!ftrace_prototype_hash))
+ goto failed;
+
+ ftrace_process_funcproto(NULL,
+ __start_funcproto,
+ __stop_funcproto,
+ false);
+#endif
+
set_ftrace_early_filters();
return;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index ad619c73a505..22433a15e340 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -940,6 +940,10 @@ extern void __trace_graph_return(struct trace_array *tr,
extern struct ftrace_hash *ftrace_graph_hash;
extern struct ftrace_hash *ftrace_graph_notrace_hash;
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+extern struct ftrace_hash *ftrace_prototype_hash;
+#endif
+
static inline int ftrace_graph_addr(struct ftrace_graph_ent *trace)
{
unsigned long addr = trace->func;
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 07/11] ftrace: prepare arch specific interfaces for function prototype feature
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
To record function parameter and return value, we need the arch specific
code to pass the saved register context. It is only valid if the
CONFIG_FTRACE_FUNC_PROTOTYPE feature is enabled. This patch only changes
the interfaces, real implementation will be added later.
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
arch/arm/kernel/ftrace.c | 2 +-
arch/arm64/kernel/ftrace.c | 2 +-
arch/csky/kernel/ftrace.c | 2 +-
arch/microblaze/kernel/ftrace.c | 2 +-
arch/mips/kernel/ftrace.c | 2 +-
arch/nds32/kernel/ftrace.c | 5 +++--
arch/parisc/kernel/ftrace.c | 2 +-
arch/powerpc/kernel/trace/ftrace.c | 2 +-
arch/riscv/kernel/ftrace.c | 2 +-
arch/s390/kernel/ftrace.c | 2 +-
arch/sh/kernel/ftrace.c | 2 +-
arch/sparc/kernel/ftrace.c | 2 +-
arch/x86/kernel/ftrace.c | 2 +-
include/linux/ftrace.h | 10 +++++++---
kernel/trace/fgraph.c | 21 +++++++++++++++------
kernel/trace/ftrace.c | 4 +++-
kernel/trace/trace.h | 2 +-
kernel/trace/trace_functions_graph.c | 2 +-
kernel/trace/trace_irqsoff.c | 3 ++-
kernel/trace/trace_sched_wakeup.c | 3 ++-
20 files changed, 46 insertions(+), 28 deletions(-)
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index bda949fd84e8..fd01c08b2dcb 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -191,7 +191,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
old = *parent;
*parent = return_hooker;
- if (function_graph_enter(old, self_addr, frame_pointer, NULL))
+ if (function_graph_enter(old, self_addr, frame_pointer, NULL, NULL))
*parent = old;
}
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 171773257974..dc8cc516c00a 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -233,7 +233,7 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
*/
old = *parent;
- if (!function_graph_enter(old, self_addr, frame_pointer, NULL))
+ if (!function_graph_enter(old, self_addr, frame_pointer, NULL, NULL))
*parent = return_hooker;
}
diff --git a/arch/csky/kernel/ftrace.c b/arch/csky/kernel/ftrace.c
index 44f4880179b7..5bc67f447e78 100644
--- a/arch/csky/kernel/ftrace.c
+++ b/arch/csky/kernel/ftrace.c
@@ -148,7 +148,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
old = *parent;
if (!function_graph_enter(old, self_addr,
- *(unsigned long *)frame_pointer, parent)) {
+ *(unsigned long *)frame_pointer, parent, NULL)) {
/*
* For csky-gcc function has sub-call:
* subi sp, sp, 8
diff --git a/arch/microblaze/kernel/ftrace.c b/arch/microblaze/kernel/ftrace.c
index 224eea40e1ee..9722e98cd01d 100644
--- a/arch/microblaze/kernel/ftrace.c
+++ b/arch/microblaze/kernel/ftrace.c
@@ -62,7 +62,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
return;
}
- if (function_graph_enter(old, self_addr, 0, NULL))
+ if (function_graph_enter(old, self_addr, 0, NULL, NULL))
*parent = old;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 2625232bfe52..24668bf079d2 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -378,7 +378,7 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
insns = core_kernel_text(self_ra) ? 2 : MCOUNT_OFFSET_INSNS + 1;
self_ra -= (MCOUNT_INSN_SIZE * insns);
- if (function_graph_enter(old_parent_ra, self_ra, fp, NULL))
+ if (function_graph_enter(old_parent_ra, self_ra, fp, NULL, NULL))
*parent_ra_addr = old_parent_ra;
return;
out:
diff --git a/arch/nds32/kernel/ftrace.c b/arch/nds32/kernel/ftrace.c
index fd2a54b8cd57..3dbf0017dfdf 100644
--- a/arch/nds32/kernel/ftrace.c
+++ b/arch/nds32/kernel/ftrace.c
@@ -217,7 +217,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
old = *parent;
- if (!function_graph_enter(old, self_addr, frame_pointer, NULL))
+ if (!function_graph_enter(old, self_addr, frame_pointer, NULL, NULL))
*parent = return_hooker;
}
@@ -235,7 +235,8 @@ noinline void ftrace_graph_caller(void)
prepare_ftrace_return(parent_ip, selfpc, frame_pointer);
}
-extern unsigned long ftrace_return_to_handler(unsigned long frame_pointer);
+extern unsigned long ftrace_return_to_handler(unsigned long frame_pointer,
+ unsigned long retval);
void __naked return_to_handler(void)
{
__asm__ __volatile__ (
diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c
index b6fb30f2e4bf..ea02f36e4f84 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -40,7 +40,7 @@ static void __hot prepare_ftrace_return(unsigned long *parent,
old = *parent;
- if (!function_graph_enter(old, self_addr, 0, NULL))
+ if (!function_graph_enter(old, self_addr, 0, NULL, NULL))
/* activate parisc_return_to_handler() as return point */
*parent = (unsigned long) &parisc_return_to_handler;
}
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index be1ca98fce5c..78174bbb257e 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -956,7 +956,7 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip)
return_hooker = ppc_function_entry(return_to_handler);
- if (!function_graph_enter(parent, ip, 0, NULL))
+ if (!function_graph_enter(parent, ip, 0, NULL, NULL))
parent = return_hooker;
out:
return parent;
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index b94d8db5ddcc..18f836727950 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -142,7 +142,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
*/
old = *parent;
- if (function_graph_enter(old, self_addr, frame_pointer, parent))
+ if (function_graph_enter(old, self_addr, frame_pointer, parent, NULL))
*parent = return_hooker;
}
diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
index 1bb85f60c0dd..5021a23c5089 100644
--- a/arch/s390/kernel/ftrace.c
+++ b/arch/s390/kernel/ftrace.c
@@ -209,7 +209,7 @@ unsigned long prepare_ftrace_return(unsigned long ra, unsigned long sp,
if (unlikely(atomic_read(¤t->tracing_graph_pause)))
goto out;
ip -= MCOUNT_INSN_SIZE;
- if (!function_graph_enter(ra, ip, 0, (void *) sp))
+ if (!function_graph_enter(ra, ip, 0, (void *) sp), NULL)
ra = (unsigned long) return_to_handler;
out:
return ra;
diff --git a/arch/sh/kernel/ftrace.c b/arch/sh/kernel/ftrace.c
index 1b04270e5460..3a8271993e9c 100644
--- a/arch/sh/kernel/ftrace.c
+++ b/arch/sh/kernel/ftrace.c
@@ -364,7 +364,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
return;
}
- if (function_graph_enter(old, self_addr, 0, NULL))
+ if (function_graph_enter(old, self_addr, 0, NULL, NULL))
__raw_writel(old, parent);
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/sparc/kernel/ftrace.c b/arch/sparc/kernel/ftrace.c
index 684b84ce397f..2783185719ba 100644
--- a/arch/sparc/kernel/ftrace.c
+++ b/arch/sparc/kernel/ftrace.c
@@ -130,7 +130,7 @@ unsigned long prepare_ftrace_return(unsigned long parent,
if (unlikely(atomic_read(¤t->tracing_graph_pause)))
return parent + 8UL;
- if (function_graph_enter(parent, self_addr, frame_pointer, NULL))
+ if (function_graph_enter(parent, self_addr, frame_pointer, NULL, NULL))
return parent + 8UL;
return return_hooker;
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 024c3053dbba..a044734167af 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -1072,7 +1072,7 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
return;
}
- if (function_graph_enter(old, self_addr, frame_pointer, parent))
+ if (function_graph_enter(old, self_addr, frame_pointer, parent, NULL))
*parent = old;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index f5aab37a8c34..e615b5e639aa 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -757,9 +757,12 @@ struct ftrace_graph_ret {
/* Type of the callback handlers for tracing function graph*/
typedef void (*trace_func_graph_ret_t)(struct ftrace_graph_ret *); /* return */
-typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *); /* entry */
+/* @pt_regs is only available for CONFIG_FTRACE_FUNC_PROTOTYPE. */
+typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *,
+ struct pt_regs *); /* entry */
-extern int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace);
+int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace,
+ struct pt_regs *pt_regs);
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
@@ -797,7 +800,8 @@ extern void return_to_handler(void);
extern int
function_graph_enter(unsigned long ret, unsigned long func,
- unsigned long frame_pointer, unsigned long *retp);
+ unsigned long frame_pointer, unsigned long *retp,
+ struct pt_regs *pt_regs);
struct ftrace_ret_stack *
ftrace_graph_get_ret_stack(struct task_struct *task, int idx);
diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 8dfd5021b933..7451dba84fee 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -96,8 +96,13 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func,
return 0;
}
+/*
+ * Called from arch specific code. @pt_regs is only available for
+ * CONFIG_FTRACE_FUNC_PROTOTYPE.
+ */
int function_graph_enter(unsigned long ret, unsigned long func,
- unsigned long frame_pointer, unsigned long *retp)
+ unsigned long frame_pointer, unsigned long *retp,
+ struct pt_regs *pt_regs)
{
struct ftrace_graph_ent trace;
@@ -108,7 +113,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,
goto out;
/* Only trace if the calling function expects to */
- if (!ftrace_graph_entry(&trace))
+ if (!ftrace_graph_entry(&trace, pt_regs))
goto out_ret;
return 0;
@@ -204,9 +209,11 @@ static struct notifier_block ftrace_suspend_notifier = {
/*
* Send the trace to the ring-buffer.
+ * @retval is only available for CONFIG_FTRACE_FUNC_PROTOTYPE.
* @return the original return address.
*/
-unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
+unsigned long ftrace_return_to_handler(unsigned long frame_pointer,
+ unsigned long retval)
{
struct ftrace_graph_ret trace;
unsigned long ret;
@@ -327,7 +334,8 @@ void ftrace_graph_sleep_time_control(bool enable)
fgraph_sleep_time = enable;
}
-int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace)
+int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace,
+ struct pt_regs *pt_regs)
{
return 0;
}
@@ -417,11 +425,12 @@ ftrace_graph_probe_sched_switch(void *ignore, bool preempt,
next->ret_stack[index].calltime += timestamp;
}
-static int ftrace_graph_entry_test(struct ftrace_graph_ent *trace)
+static int ftrace_graph_entry_test(struct ftrace_graph_ent *trace,
+ struct pt_regs *pt_regs)
{
if (!ftrace_ops_test(&global_ops, trace->func, NULL))
return 0;
- return __ftrace_graph_entry(trace);
+ return __ftrace_graph_entry(trace, pt_regs);
}
/*
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 438b8b47198f..a1683cc55838 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -794,7 +794,9 @@ void ftrace_graph_graph_time_control(bool enable)
fgraph_graph_time = enable;
}
-static int profile_graph_entry(struct ftrace_graph_ent *trace)
+/* @pt_regs is only available for CONFIG_FTRACE_FUNC_PROTOTYPE. */
+static int profile_graph_entry(struct ftrace_graph_ent *trace,
+ struct pt_regs *pt_regs)
{
struct ftrace_ret_stack *ret_stack;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 22433a15e340..4b31176d443e 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -737,7 +737,7 @@ void print_trace_header(struct seq_file *m, struct trace_iterator *iter);
int trace_empty(struct trace_iterator *iter);
void trace_graph_return(struct ftrace_graph_ret *trace);
-int trace_graph_entry(struct ftrace_graph_ent *trace);
+int trace_graph_entry(struct ftrace_graph_ent *trace, struct pt_regs *pt_regs);
void set_graph_array(struct trace_array *tr);
void tracing_start_cmdline_record(void);
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 78af97163147..f331a9ba946d 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -124,7 +124,7 @@ static inline int ftrace_graph_ignore_irqs(void)
return in_irq();
}
-int trace_graph_entry(struct ftrace_graph_ent *trace)
+int trace_graph_entry(struct ftrace_graph_ent *trace, struct pt_regs *pt_regs)
{
struct trace_array *tr = graph_array;
struct trace_array_cpu *data;
diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
index a745b0cee5d3..513e3544a45a 100644
--- a/kernel/trace/trace_irqsoff.c
+++ b/kernel/trace/trace_irqsoff.c
@@ -172,7 +172,8 @@ static int irqsoff_display_graph(struct trace_array *tr, int set)
return start_irqsoff_tracer(irqsoff_trace, set);
}
-static int irqsoff_graph_entry(struct ftrace_graph_ent *trace)
+static int irqsoff_graph_entry(struct ftrace_graph_ent *trace,
+ struct pt_regs *pt_regs)
{
struct trace_array *tr = irqsoff_trace;
struct trace_array_cpu *data;
diff --git a/kernel/trace/trace_sched_wakeup.c b/kernel/trace/trace_sched_wakeup.c
index 743b2b520d34..ce18f679930c 100644
--- a/kernel/trace/trace_sched_wakeup.c
+++ b/kernel/trace/trace_sched_wakeup.c
@@ -112,7 +112,8 @@ static int wakeup_display_graph(struct trace_array *tr, int set)
return start_func_tracer(tr, set);
}
-static int wakeup_graph_entry(struct ftrace_graph_ent *trace)
+static int wakeup_graph_entry(struct ftrace_graph_ent *trace,
+ struct pt_regs *pt_regs)
{
struct trace_array *tr = wakeup_trace;
struct trace_array_cpu *data;
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 08/11] ftrace: introduce core part of function prototype recording
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
This patch introduces the core part of our new CONFIG_FTRACE_FUNC_PROTOTYPE
feature. For arch which supports this feature must implement a new
arch-specific interface arch_fgraph_record_params().
In this patch, we add a new trace option "record-funcproto", and by now
only function graph tracer is supported. The major work is to handle
the printing stuff.
Here is an example of the graph trace of function pick_next_task_fair().
Note that we only record the parameter and return value of global
functions.
2) | pick_next_task_fair() {
2) | update_blocked_averages() {
2) 0.765 us | _raw_spin_lock_irqsave(lock=0xffff88807da2b100); /* ret=0x0000000000000082 */
2) 0.944 us | update_rq_clock(rq=0xffff88807da2b100);
2) 0.612 us | __update_load_avg_cfs_rq(now=0x000000251b8516ee, cfs_rq=0xffff8880754f7488); /* ret=0 */
2) 0.654 us | __update_load_avg_se(now=0x000000251b8516ee, cfs_rq=0xffff88807da2b180, se=0xffff88807be2e0d8); /* ret=0 */
2) 0.206 us | __update_load_avg_cfs_rq(now=0x000000251b8516ee, cfs_rq=0xffff88807da2b180); /* ret=0 */
2) | __update_load_avg_cfs_rq(now=0x000000251b8516ee, cfs_rq=0xffff888079b5fb18) {
2) 2.410 us | __accumulate_pelt_segments();
2) 3.103 us | } /* ret=1 */
2) 0.193 us | __update_load_avg_cfs_rq(now=0x000000251b8516ee, cfs_rq=0xffff88807da2b180); /* ret=0 */
2) | update_rt_rq_load_avg(now=0x000000251b8516ee, rq=0xffff88807da2b100, running=0) {
2) 0.258 us | __accumulate_pelt_segments();
2) 1.617 us | } /* ret=1 */
2) | update_dl_rq_load_avg(now=0x000000251b8516ee, rq=0xffff88807da2b100, running=0) {
2) 0.230 us | __accumulate_pelt_segments();
2) 1.511 us | } /* ret=1 */
2) 1.040 us | _raw_spin_unlock_irqrestore(lock=0xffff88807da2b100, flags=0x0000000000000082);
2) + 14.739 us | }
2) | load_balance() {
2) | find_busiest_group() {
2) 0.874 us | update_group_capacity(sd=0xffff88807c1d37d0, cpu=2);
2) 1.761 us | idle_cpu();
2) 0.262 us | idle_cpu();
2) 0.217 us | idle_cpu();
2) 6.338 us | }
2) 8.442 us | }
2) 1.823 us | __msecs_to_jiffies(m=0x00000006); /* ret=0x0000000000000002 */
2) | load_balance() {
2) | find_busiest_group() {
2) 0.434 us | idle_cpu();
2) 0.233 us | idle_cpu();
2) 0.210 us | idle_cpu();
2) 2.308 us | }
2) 2.821 us | }
2) 0.263 us | __msecs_to_jiffies(m=0x00000008); /* ret=0x0000000000000002 */
2) 0.977 us | _raw_spin_lock(lock=0xffff88807da2b100);
2) + 32.262 us | }
The printing rules of each value is:
o For signed value, it is always printed as decimal number.
o For unsigned value,
- For value has size great than 8, it is printed as '{..}'.
- For value has size of 1,2,4,8, it is printed as hexadecimal number.
- If failed to record a parameter, it is printed as '?'.
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
include/linux/ftrace.h | 27 +++++++
kernel/trace/fgraph.c | 5 ++
kernel/trace/ftrace.c | 50 +++++++++++++
kernel/trace/trace.h | 8 ++
kernel/trace/trace_entries.h | 10 +++
kernel/trace/trace_functions_graph.c | 106 +++++++++++++++++++++++++--
6 files changed, 201 insertions(+), 5 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index e615b5e639aa..82b92d355431 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -17,6 +17,7 @@
#include <linux/types.h>
#include <linux/init.h>
#include <linux/fs.h>
+#include <linux/trace_seq.h>
#include <asm/ftrace.h>
@@ -377,6 +378,9 @@ struct func_prototype {
#define FTRACE_PROTOTYPE_SIGNED(t) (t & BIT(7))
#define FTRACE_PROTOTYPE_SIZE(t) (t & GENMASK(6, 0))
+
+void ftrace_print_typed_val(struct trace_seq *s, uint8_t type,
+ unsigned long val);
#endif
int ftrace_force_update(void);
@@ -731,6 +735,13 @@ extern void ftrace_init(void);
static inline void ftrace_init(void) { }
#endif
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+# define FTRACE_MAX_FUNC_PARAMS 10
+
+# define FTRACE_PROTOTYPE_SIGNED(t) (t & BIT(7))
+# define FTRACE_PROTOTYPE_SIZE(t) (t & GENMASK(6, 0))
+#endif
+
/*
* Structure that defines an entry function trace.
* It's already packed but the attribute "packed" is needed
@@ -739,6 +750,12 @@ static inline void ftrace_init(void) { }
struct ftrace_graph_ent {
unsigned long func; /* Current function */
int depth;
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ uint8_t nr_param;
+ char *param_names[FTRACE_MAX_FUNC_PARAMS];
+ uint8_t param_types[FTRACE_MAX_FUNC_PARAMS];
+ unsigned long param_values[FTRACE_MAX_FUNC_PARAMS];
+#endif
} __packed;
/*
@@ -753,8 +770,13 @@ struct ftrace_graph_ret {
unsigned long long calltime;
unsigned long long rettime;
int depth;
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ uint8_t ret_type;
+ unsigned long retval;
+#endif
} __packed;
+
/* Type of the callback handlers for tracing function graph*/
typedef void (*trace_func_graph_ret_t)(struct ftrace_graph_ret *); /* return */
/* @pt_regs is only available for CONFIG_FTRACE_FUNC_PROTOTYPE. */
@@ -842,6 +864,11 @@ static inline void unpause_graph_tracing(void)
{
atomic_dec(¤t->tracing_graph_pause);
}
+
+void arch_fgraph_record_params(struct ftrace_graph_ent *trace,
+ struct func_prototype *proto,
+ struct pt_regs *pt_regs);
+
#else /* !CONFIG_FUNCTION_GRAPH_TRACER */
#define __notrace_funcgraph
diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 7451dba84fee..26e452418249 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -220,6 +220,11 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer,
ftrace_pop_return_trace(&trace, &ret, frame_pointer);
trace.rettime = trace_clock_local();
+
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ trace.retval = retval;
+#endif
+
ftrace_graph_return(&trace);
/*
* The ftrace_graph_return() may still access the current
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a1683cc55838..1e6a96f1986b 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5658,6 +5658,56 @@ static int ftrace_process_funcproto(struct module *mod,
return ret;
}
+
+void ftrace_print_typed_val(struct trace_seq *s, uint8_t type,
+ unsigned long val)
+{
+ unsigned int sz = FTRACE_PROTOTYPE_SIZE(type);
+ bool is_signed = FTRACE_PROTOTYPE_SIGNED(type);
+
+ /* Don't show complex types */
+ if (sz > sizeof(long)) {
+ trace_seq_printf(s, "{..}");
+ return;
+ }
+
+ switch (sz) {
+ case 0:
+ /* The value is not valid. */
+ trace_seq_printf(s, "?");
+ break;
+ case 1:
+ val &= GENMASK_ULL(7, 0);
+ if (is_signed)
+ trace_seq_printf(s, "%d", (char)val);
+ else
+ trace_seq_printf(s, "0x%02lx", val);
+ break;
+ case 2:
+ val &= GENMASK_ULL(15, 0);
+ if (is_signed)
+ trace_seq_printf(s, "%d", (short)val);
+ else
+ trace_seq_printf(s, "0x%04lx", val);
+ break;
+ case 4:
+ val &= GENMASK_ULL(31, 0);
+ if (is_signed)
+ trace_seq_printf(s, "%d", (int)val);
+ else
+ trace_seq_printf(s, "0x%08lx", val);
+ break;
+ case 8:
+ val &= GENMASK_ULL(63, 0);
+ if (is_signed)
+ trace_seq_printf(s, "%lld", (long long)val);
+ else
+ trace_seq_printf(s, "0x%016lx", val);
+ break;
+ default:
+ trace_seq_printf(s, "{badsize%d}", sz);
+ }
+}
#endif
struct ftrace_mod_func {
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 4b31176d443e..f10acad0140f 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1231,6 +1231,13 @@ extern int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
# define STACK_FLAGS
#endif
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+# define FUNCPROTO_FLAGS \
+ C(RECORD_FUNCPROTO, "record-funcproto"),
+#else
+# define FUNCPROTO_FLAGS
+#endif
+
/*
* trace_iterator_flags is an enumeration that defines bit
* positions into trace_flags that controls the output.
@@ -1256,6 +1263,7 @@ extern int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
C(LATENCY_FMT, "latency-format"), \
C(RECORD_CMD, "record-cmd"), \
C(RECORD_TGID, "record-tgid"), \
+ FUNCPROTO_FLAGS \
C(OVERWRITE, "overwrite"), \
C(STOP_ON_FREE, "disable_on_free"), \
C(IRQ_INFO, "irq-info"), \
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index fc8e97328e54..68b044ea8440 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -82,6 +82,12 @@ FTRACE_ENTRY_PACKED(funcgraph_entry, ftrace_graph_ent_entry,
__field_struct( struct ftrace_graph_ent, graph_ent )
__field_desc( unsigned long, graph_ent, func )
__field_desc( int, graph_ent, depth )
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ __field_desc( unsigned char, graph_ent, nr_param )
+ __array_desc( char *, graph_ent, param_names, FTRACE_MAX_FUNC_PARAMS)
+ __array_desc( uint8_t, graph_ent, param_types, FTRACE_MAX_FUNC_PARAMS)
+ __array_desc( unsigned long, graph_ent, param_values, FTRACE_MAX_FUNC_PARAMS)
+#endif
),
F_printk("--> %ps (%d)", (void *)__entry->func, __entry->depth),
@@ -101,6 +107,10 @@ FTRACE_ENTRY_PACKED(funcgraph_exit, ftrace_graph_ret_entry,
__field_desc( unsigned long long, ret, rettime )
__field_desc( unsigned long, ret, overrun )
__field_desc( int, ret, depth )
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ __field_desc( unsigned char, ret, ret_type)
+ __field_desc( unsigned long, ret, retval )
+#endif
),
F_printk("<-- %ps (%d) (start: %llx end: %llx) over: %d",
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index f331a9ba946d..ba4eb71646e9 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -169,6 +169,17 @@ int trace_graph_entry(struct ftrace_graph_ent *trace, struct pt_regs *pt_regs)
if (tracing_thresh)
return 1;
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ trace->nr_param = 0;
+ if (tr->trace_flags & TRACE_ITER_RECORD_FUNCPROTO) {
+ struct ftrace_func_entry *ent;
+
+ ent = ftrace_lookup_ip(ftrace_prototype_hash, trace->func);
+ if (ent)
+ arch_fgraph_record_params(trace, ent->priv, pt_regs);
+ }
+#endif
+
local_irq_save(flags);
cpu = raw_smp_processor_id();
data = per_cpu_ptr(tr->trace_buffer.data, cpu);
@@ -250,6 +261,21 @@ void trace_graph_return(struct ftrace_graph_ret *trace)
return;
}
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+ if (tr->trace_flags & TRACE_ITER_RECORD_FUNCPROTO) {
+ struct ftrace_func_entry *ent;
+
+ ent = ftrace_lookup_ip(ftrace_prototype_hash, trace->func);
+ if (ent) {
+ /* The retval has been saved by trace_graph_return(). */
+ trace->ret_type =
+ ((struct func_prototype *)ent->priv)->ret_type;
+ } else
+ trace->ret_type = 0;
+ } else
+ trace->ret_type = 0;
+#endif
+
local_irq_save(flags);
cpu = raw_smp_processor_id();
data = per_cpu_ptr(tr->trace_buffer.data, cpu);
@@ -380,6 +406,71 @@ static void print_graph_lat_fmt(struct trace_seq *s, struct trace_entry *entry)
trace_seq_puts(s, " | ");
}
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+static void print_graph_params(struct trace_seq *s,
+ struct ftrace_graph_ent *call,
+ struct ftrace_graph_ret *graph_ret)
+{
+ int i;
+
+ BUG_ON(call->nr_param > FTRACE_MAX_FUNC_PARAMS);
+
+ trace_seq_printf(s, "%ps(", (void *)call->func);
+ for (i = 0; i < call->nr_param; i++) {
+ if (i > 0)
+ trace_seq_printf(s, ", ");
+ trace_seq_printf(s, "%s=", call->param_names[i]);
+ ftrace_print_typed_val(s, call->param_types[i],
+ call->param_values[i]);
+ }
+
+ if (graph_ret) {
+ /* leaf */
+ if (graph_ret->ret_type) {
+ trace_seq_printf(s, "); /* ret=");
+ ftrace_print_typed_val(s, graph_ret->ret_type,
+ graph_ret->retval);
+ trace_seq_puts(s, " */\n");
+ } else
+ trace_seq_puts(s, ");\n");
+ } else
+ trace_seq_printf(s, ") {\n");
+}
+
+static void print_graph_retval(struct trace_seq *s,
+ struct ftrace_graph_ret *trace,
+ bool tail)
+{
+ if (trace->ret_type) {
+ if (tail)
+ trace_seq_puts(s, ", ");
+ else
+ trace_seq_puts(s, " /* ");
+
+ trace_seq_printf(s, "ret=");
+ ftrace_print_typed_val(s, trace->ret_type, trace->retval);
+
+ trace_seq_printf(s, " */");
+ }
+}
+#else
+static void print_graph_params(struct trace_seq *s,
+ struct ftrace_graph_ent *call,
+ struct ftrace_graph_ret *graph_ret)
+{
+ if (graph_ret)
+ trace_seq_printf(s, "%ps();\n", (void *)call->func);
+ else
+ trace_seq_printf(s, "%ps() {\n", (void *)call->func);
+}
+
+static void print_graph_retval(struct trace_seq *s,
+ struct ftrace_graph_ret *trace,
+ bool tail)
+{
+}
+#endif
+
/* If the pid changed since the last trace, output this event */
static void
verif_pid(struct trace_seq *s, pid_t pid, int cpu, struct fgraph_data *data)
@@ -665,7 +756,7 @@ print_graph_entry_leaf(struct trace_iterator *iter,
for (i = 0; i < call->depth * TRACE_GRAPH_INDENT; i++)
trace_seq_putc(s, ' ');
- trace_seq_printf(s, "%ps();\n", (void *)call->func);
+ print_graph_params(s, call, graph_ret);
print_graph_irq(iter, graph_ret->func, TRACE_GRAPH_RET,
cpu, iter->ent->pid, flags);
@@ -703,7 +794,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
for (i = 0; i < call->depth * TRACE_GRAPH_INDENT; i++)
trace_seq_putc(s, ' ');
- trace_seq_printf(s, "%ps() {\n", (void *)call->func);
+ print_graph_params(s, call, NULL);
if (trace_seq_has_overflowed(s))
return TRACE_TYPE_PARTIAL_LINE;
@@ -950,10 +1041,15 @@ print_graph_return(struct ftrace_graph_ret *trace, struct trace_seq *s,
* belongs to, write out the function name. Always do
* that if the funcgraph-tail option is enabled.
*/
- if (func_match && !(flags & TRACE_GRAPH_PRINT_TAIL))
- trace_seq_puts(s, "}\n");
- else
+ if (func_match && !(flags & TRACE_GRAPH_PRINT_TAIL)) {
+ trace_seq_puts(s, "}");
+ print_graph_retval(s, trace, false);
+ trace_seq_puts(s, "\n");
+ } else {
trace_seq_printf(s, "} /* %ps */\n", (void *)trace->func);
+ print_graph_retval(s, trace, true);
+ trace_seq_puts(s, "\n");
+ }
/* Overrun */
if (flags & TRACE_GRAPH_PRINT_OVERRUN)
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 09/11] x86_64: add function prototype recording support
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
This patch implements the arch_fgraph_record_params() function for x86_64
platform and deliver the return value of function to ftrace core part.
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
arch/x86/Kconfig | 1 +
arch/x86/kernel/ftrace.c | 84 +++++++++++++++++++++++++++++++++++--
arch/x86/kernel/ftrace_64.S | 4 +-
3 files changed, 85 insertions(+), 4 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 222855cc0158..34e583bfdab8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -31,6 +31,7 @@ config X86_64
select NEED_DMA_MAP_STATE
select SWIOTLB
select ARCH_HAS_SYSCALL_WRAPPER
+ select HAVE_FTRACE_FUNC_PROTOTYPE
config FORCE_DYNAMIC_FTRACE
def_bool y
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index a044734167af..fc0a062ce762 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -31,6 +31,7 @@
#include <asm/ftrace.h>
#include <asm/nops.h>
#include <asm/text-patching.h>
+#include <asm-generic/dwarf.h>
#ifdef CONFIG_DYNAMIC_FTRACE
@@ -918,7 +919,8 @@ static void *addr_from_call(void *ptr)
}
void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
- unsigned long frame_pointer);
+ unsigned long frame_pointer,
+ struct pt_regs *pt_regs);
/*
* If the ops->trampoline was not allocated, then it probably
@@ -973,6 +975,82 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
ops->trampoline = 0;
}
+#ifdef CONFIG_FTRACE_FUNC_PROTOTYPE
+void arch_fgraph_record_params(struct ftrace_graph_ent *trace,
+ struct func_prototype *proto,
+ struct pt_regs *pt_regs)
+{
+ int i;
+
+ trace->nr_param = min(proto->nr_param, (uint8_t)FTRACE_MAX_FUNC_PARAMS);
+
+ for (i = 0; i < trace->nr_param; i++) {
+ struct func_param *param = &proto->params[i];
+ unsigned int sz = FTRACE_PROTOTYPE_SIZE(param->type);
+ long off = (char)param->loc[1];
+ unsigned long value = 0;
+ bool good = true;
+
+ if (sz > sizeof(value)) {
+ /* Don't record value of complex type. */
+ trace->param_types[i] = param->type;
+ trace->param_values[i] = 0;
+ continue;
+ }
+
+ switch (param->loc[0]) {
+ case DW_OP_reg1:
+ value = pt_regs->dx;
+ break;
+ case DW_OP_reg2:
+ value = pt_regs->cx;
+ break;
+ case DW_OP_reg3:
+ value = pt_regs->bx;
+ break;
+ case DW_OP_reg4:
+ value = pt_regs->si;
+ break;
+ case DW_OP_reg5:
+ value = pt_regs->di;
+ break;
+ case DW_OP_reg6:
+ value = pt_regs->bp;
+ break;
+ case DW_OP_reg8:
+ value = pt_regs->r8;
+ break;
+ case DW_OP_reg9:
+ value = pt_regs->r9;
+ break;
+ case DW_OP_fbreg:
+ if (probe_kernel_read(&value,
+ (void *)pt_regs->bp + off,
+ sz))
+ good = false;
+ break;
+ case DW_OP_breg7:
+ if (probe_kernel_read(&value,
+ (void *)pt_regs->sp + off,
+ sz))
+ good = false;
+ break;
+ default:
+ /* unexpected loc expression */
+ good = false;
+ }
+
+ trace->param_names[i] = param->name;
+ if (good) {
+ trace->param_types[i] = param->type;
+ trace->param_values[i] = value;
+ } else {
+ /* set the type to 0 so we skip it when printing. */
+ trace->param_types[i] = 0;
+ }
+ }
+}
+#endif /* CONFIG_FTRACE_FUNC_PROTOTYPE */
#endif /* CONFIG_X86_64 */
#endif /* CONFIG_DYNAMIC_FTRACE */
@@ -1017,7 +1095,7 @@ int ftrace_disable_ftrace_graph_caller(void)
* in current thread info.
*/
void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
- unsigned long frame_pointer)
+ unsigned long frame_pointer, struct pt_regs *pt_regs)
{
unsigned long old;
int faulted;
@@ -1072,7 +1150,7 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
return;
}
- if (function_graph_enter(old, self_addr, frame_pointer, parent, NULL))
+ if (function_graph_enter(old, self_addr, frame_pointer, parent, pt_regs))
*parent = old;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 809d54397dba..e01d6358e859 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -289,7 +289,8 @@ ENTRY(ftrace_graph_caller)
leaq MCOUNT_REG_SIZE+8(%rsp), %rsi
movq $0, %rdx /* No framepointers needed */
- call prepare_ftrace_return
+ movq %rsp, %rcx /* the fourth parameter */
+ call prepare_ftrace_return
restore_mcount_regs
@@ -304,6 +305,7 @@ ENTRY(return_to_handler)
movq %rax, (%rsp)
movq %rdx, 8(%rsp)
movq %rbp, %rdi
+ movq %rax, %rsi
call ftrace_return_to_handler
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 10/11] ftrace: add doc for new option record-funcproto
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
Just add the doc for our new feature.
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
Documentation/trace/ftrace.rst | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
index f60079259669..c68fbbedb8bd 100644
--- a/Documentation/trace/ftrace.rst
+++ b/Documentation/trace/ftrace.rst
@@ -988,6 +988,7 @@ To see what is available, simply cat the file::
nolatency-format
record-cmd
norecord-tgid
+ norecord-funcproto
overwrite
nodisable_on_free
irq-info
@@ -1131,6 +1132,11 @@ Here are the available options:
mapped Thread Group IDs (TGID) mapping to pids. See
"saved_tgids".
+ record-funcproto
+ Record function parameters and return value. This option
+ is only supported by function_graph tracer on x86_64
+ platform by now.
+
overwrite
This controls what happens when the trace buffer is
full. If "1" (default), the oldest events are
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 11/11] MAINTAINERS: make scripts/ftrace/ maintained
From: Changbin Du @ 2019-08-25 13:23 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar
Cc: linux-arch, Jonathan Corbet, linux-parisc, linux-doc, linux-sh,
linux-s390, x86, linux-kernel, linux-mips, Jessica Yu, sparclinux,
linux-kbuild, Thomas Gleixner, linuxppc-dev, linux-riscv,
linux-arm-kernel, Changbin Du
In-Reply-To: <20190825132330.5015-1-changbin.du@gmail.com>
Make scripts/ftrace/ maintained and I would like to help with reviewing
related patches.
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 9cbcf167bdd0..ca012ea260d7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16293,6 +16293,7 @@ F: drivers/char/tpm/
TRACING
M: Steven Rostedt <rostedt@goodmis.org>
M: Ingo Molnar <mingo@redhat.com>
+R: Changbin Du <changbin.du@gmail.com>
T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git perf/core
S: Maintained
F: Documentation/trace/ftrace.rst
@@ -16303,6 +16304,7 @@ F: include/linux/trace*.h
F: include/trace/
F: kernel/trace/
F: tools/testing/selftests/ftrace/
+F: scripts/ftrace/
TRACING MMIO ACCESSES (MMIOTRACE)
M: Steven Rostedt <rostedt@goodmis.org>
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [GIT PULL] ARM: aspeed: arch changes for 5.4
From: Joel Stanley @ 2019-08-25 14:09 UTC (permalink / raw)
To: arm, soc; +Cc: Andrew Jeffery, Linux ARM, linux-aspeed
Hello ARM Maintainers,
Here's my first mach-aspeed pull request. We finally had to add some
code here to support SMP on the shiny new ASPEED AST2600.
The following changes since commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b:
Linus 5.3-rc1 (2019-07-21 14:05:38 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/joel/aspeed.git \
tags/aspeed-5.4-arch
for you to fetch changes up to 87dfe49691a3aefd66ebe76a4a0cc9e872d2587b:
ARM: aspeed: Enable SMP boot (2019-08-25 23:26:52 +0930)
----------------------------------------------------------------
ASPEED architecture updates for 5.4
This adds support for the new ASPEED AST2600 BMC SoC.
----------------------------------------------------------------
Joel Stanley (4):
dt-bindings: arm: cpus: Add ASPEED SMP
ARM: aspeed: Select timer in each SoC
ARM: aspeed: Add ASPEED AST2600 architecture
ARM: aspeed: Enable SMP boot
Documentation/devicetree/bindings/arm/cpus.yaml | 1 +
arch/arm/Makefile | 1 +
arch/arm/mach-aspeed/Kconfig | 17 ++++++-
arch/arm/mach-aspeed/Makefile | 5 ++
arch/arm/mach-aspeed/platsmp.c | 61 +++++++++++++++++++++++++
5 files changed, 83 insertions(+), 2 deletions(-)
create mode 100644 arch/arm/mach-aspeed/Makefile
create mode 100644 arch/arm/mach-aspeed/platsmp.c
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [GIT PULL] ARM: aspeed: devicetree changes for 5.4
From: Joel Stanley @ 2019-08-25 14:10 UTC (permalink / raw)
To: arm, soc; +Cc: Andrew Jeffery, Linux ARM, linux-aspeed
Hello ARM Maintainers,
Here are the APSEED device tree changes. No ast2600 support here
unfortunately as the clock driver wasn't quite ready in time.
The following changes since commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b:
Linus 5.3-rc1 (2019-07-21 14:05:38 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/joel/aspeed.git \
tags/aspeed-5.4-devicetree
for you to fetch changes up to 49b0f3be0b86292eed6f6aedadf4252131d9c111:
ARM: dts: aspeed: swift: Add eMMC device (2019-08-22 15:34:20 +0930)
----------------------------------------------------------------
ASPEED device tree updates for 5.4
New machines:
- Facebook Wedge100, Wedge40 and Minipack
- Lenovo Hr855xg2
- Wistron Mihawk
There's a few other updates, notably some changes to to use the newly
added SDHCI driver.
----------------------------------------------------------------
Andrew Jeffery (2):
ARM: dts: aspeed: Describe SD controllers
ARM: dts: aspeed: Enable first MMC slot on AST2500 EVB
Andrew Peng (1):
ARM: dts: aspeed: Add Lenovo Hr855xg2 BMC
Ben Pai (1):
ARM: dts: aspeed: Add Mihawk BMC platform
Hongwei Zhang (1):
ARM: dts: aspeed: Add SGPM pinmux
Joel Stanley (1):
ARM: dts: aspeed: swift: Add eMMC device
John Wang (1):
ARM: dts: aspeed: fp5280g2: Fix power supply address
Matt Spinler (1):
ARM: dts: aspeed: swift: Fix FSI GPIOs
Tao Ren (3):
ARM: dts: aspeed: Add Facebook Minipack BMC
ARM: dts: aspeed: Add Facebook Wedge40 BMC
ARM: dts: aspeed: Add Facebook Wedge100 BMC
Vijay Khemka (3):
ARM: dts: aspeed: tiogapass: Add VR devices
ARM: dts: aspeed: tiogapass: Move battery sensor
ARM: dts: aspeed: tiogapass: Add Riser card
arch/arm/boot/dts/Makefile | 5 +
arch/arm/boot/dts/aspeed-ast2500-evb.dts | 11 +
arch/arm/boot/dts/aspeed-bmc-facebook-minipack.dts | 429 ++++++++++
.../arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 272 +++++-
arch/arm/boot/dts/aspeed-bmc-facebook-wedge100.dts | 149 ++++
arch/arm/boot/dts/aspeed-bmc-facebook-wedge40.dts | 141 ++++
arch/arm/boot/dts/aspeed-bmc-inspur-fp5280g2.dts | 4 +-
arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts | 663 +++++++++++++++
arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts | 918 +++++++++++++++++++++
arch/arm/boot/dts/aspeed-bmc-opp-swift.dts | 15 +-
arch/arm/boot/dts/aspeed-g4.dtsi | 28 +
arch/arm/boot/dts/aspeed-g5.dtsi | 33 +
12 files changed, 2659 insertions(+), 9 deletions(-)
create mode 100644 arch/arm/boot/dts/aspeed-bmc-facebook-minipack.dts
create mode 100644 arch/arm/boot/dts/aspeed-bmc-facebook-wedge100.dts
create mode 100644 arch/arm/boot/dts/aspeed-bmc-facebook-wedge40.dts
create mode 100644 arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
create mode 100755 arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox