* Re: [PATCH v5 1/2] arm64: dts: fsl: pico-pi: Add a device tree for the PICO-PI-IMX8M
From: Fabio Estevam @ 2019-07-20 15:09 UTC (permalink / raw)
To: andradanciu1997
Cc: Shawn Guo, Rob Herring, Mark Rutland, Sascha Hauer, Sascha Hauer,
NXP Linux Team, Manivannan Sadhasivam, Andrey Smirnov,
Jonathan Neuschäfer, Dong Aisheng, Li Yang, Lucas Stach,
pankaj.bansal, Bhaskar Upadhaya, Pramod Kumar,
Angus Ainslie (Purism), Richard Hu,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <20190719121430.9318-2-andradanciu1997@gmail.com>
Hi Andra,
Just realized one minor issue:
On Fri, Jul 19, 2019 at 9:14 AM andradanciu1997
<andradanciu1997@gmail.com> wrote:
> +&i2c1 {
> + clock-frequency = <100000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_i2c1>;
> + status = "okay";
> +
> + pmic: pmic@4b {
> + reg = <0x4b>;
> + compatible = "rohm,bd71837";
> + /* PMIC BD71837 PMIC_nINT GPIO1_IO12 */
Comment says gpio1 12...
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_pmic>;
> + clocks = <&pmic_osc>;
> + clock-names = "osc";
> + clock-output-names = "pmic_clk";
> + interrupt-parent = <&gpio1>;
> + interrupts = <3 GPIO_ACTIVE_LOW>;
but here you use gpio1 3 instead, so there is a mismatch.
Please check against the schematics and pick the correct one.
I would suggest removing the:
/* PMIC BD71837 PMIC_nINT GPIO1_IO12 */
comment entirely.
For the next version you can:
Reviewed-by: Fabio Estevam <festevam@gmail.com>
Thanks
^ permalink raw reply
* Re: [TRIVIAL PATCH] of: per-file dtc preprocessor flags
From: Eugeniy Paltsev @ 2019-07-20 15:37 UTC (permalink / raw)
To: Eugeniy.Paltsev@synopsys.com, yamada.masahiro@socionext.com
Cc: devicetree@vger.kernel.org, michal.lkml@markovi.net,
linux-kbuild@vger.kernel.org, Vineet.Gupta1@synopsys.com,
Alexey Brodkin, linux-kernel@vger.kernel.org,
linux-snps-arc@lists.infradead.org
In-Reply-To: <CAK7LNARJjb3fgdo_dxPSv-vs5x573OHKf-UAUJGMc89Jfmk+QA@mail.gmail.com>
Hi Masahiro,
On Sat, 2019-07-20 at 11:40 +0900, Masahiro Yamada wrote:
> On Sat, Jul 20, 2019 at 4:28 AM Eugeniy Paltsev
> <Eugeniy.Paltsev@synopsys.com> wrote:
> > As of today we are able to pass custom flags to dtc compiler but not
> > to dtc preprocessor.
> > This ability is required for including some board-specific header files.
> > It allows us to pass defined constants to dts without their duplication
> > in several places.
>
> How to use this option in a useful way?
>
> I see a bunch of defined constants under include/dt-bindings/.
>
> If you are talking about code duplication across architectures,
> you can include arm dtsi from arm64 dts, or vice versa.
> This was made possible by the symbolic links
> in scripts/dtc/include-prefixes/.
>
> Could you please elaborate your issues if you cannot solve them
> by the current strategy?
>
Here is the example:
We have several FPGA-based boards (haps_hs, haps_hs_idu,...) which are involved
in SW and HW automated verification. For some tests we randomize physical memory
location so we patch 'CONFIG_LINUX_RAM_BASE' kconfig variable and 'memory' node in
device tree. We want to keep number of patches as less as possible (to avoid
conflicts on their applying) so we want to avoid duplication and pass
'CONFIG_LINUX_RAM_BASE' directly to dts by including it from
'include/generated/autoconf.h':
----------------------------->8------------------------------------
memory {
device_type = "memory";
- /* CONFIG_LINUX_RAM_BASE needs to match low mem start */
- reg = <0x80000000 0x20000000>; /* 512 */
+ reg = <CONFIG_LINUX_RAM_BASE 0x20000000>; /* 512 */
};
----------------------------->8------------------------------------
This patch allows us to extend the search path of include files to
include 'include/generated/autoconf.h'.
>
> > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> > ---
> > scripts/Makefile.lib | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index f1f38c8cdc74..f2595a608dce 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -166,6 +166,8 @@ dtc_cpp_flags = -Wp,-MD,$(depfile).pre.tmp -nostdinc \
> > $(addprefix -I,$(DTC_INCLUDE)) \
> > -undef -D__DTS__
> >
> > +dtc_cpp_flags += $(DTC_CPP_FLAGS_$(basetarget))
> > +
> > # Useful for describing the dependency of composite objects
> > # Usage:
> > # $(call multi_depend, multi_used_targets, suffix_to_remove, suffix_to_add)
> > --
> > 2.21.0
> >
>
>
--
Eugeniy Paltsev
^ permalink raw reply
* Re: [EXT] Re: [PATCH 00/10] Improvements and fixes for mxsfb DRM driver
From: Guido Günther @ 2019-07-20 21:09 UTC (permalink / raw)
To: Robert Chiras
Cc: marex@denx.de, devicetree@vger.kernel.org, kernel@pengutronix.de,
airlied@linux.ie, shawnguo@kernel.org,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
robh+dt@kernel.org, mark.rutland@arm.com,
linux-arm-kernel@lists.infradead.org, dl-linux-imx
In-Reply-To: <20190716145450.GA609@bogon.m.sigxcpu.org>
Hi Robert,
On Tue, Jul 16, 2019 at 04:54:50PM +0200, Guido Günther wrote:
> Hi Robert,
> On Fri, Jul 12, 2019 at 08:15:32AM +0000, Robert Chiras wrote:
> > Hi Guido,
> >
> > On Jo, 2019-07-11 at 17:04 +0200, Guido Günther wrote:
> > > Hi Robert,
> > > On Wed, Jun 26, 2019 at 04:32:08PM +0300, Robert Chiras wrote:
> > > >
> > > > This patch-set improves the use of eLCDIF block on iMX 8 SoCs (like
> > > > 8MQ, 8MM
> > > > and 8QXP). Following, are the new features added and fixes from
> > > > this
> > > > patch-set:
> > > >
> > > > 1. Add support for drm_bridge
> > > > On 8MQ and 8MM, the LCDIF block is not directly connected to a
> > > > parallel
> > > > display connector, where an LCD panel can be attached, but instead
> > > > it is
> > > > connected to DSI controller. Since this DSI stands between the
> > > > display
> > > > controller (eLCDIF) and the physical connector, the DSI can be
> > > > implemented
> > > > as a DRM bridge. So, in order to be able to connect the mxsfb
> > > > driver to
> > > > the DSI driver, the support for a drm_bridge was needed in mxsfb
> > > > DRM
> > > > driver (the actual driver for the eLCDIF block).
> > > So I wanted to test this but with both my somewhat cleaned up nwl
> > > driver¹ and the nwl driver forward ported from the nxp vendor tree
> > > I'm
> > > looking at a black screen with current mainline - while my dcss
> > > forward
> > > port gives me nice output on mipi dsi. Do you have a tree that uses
> > > mipi
> > > dsi on imx8mq where I could look at to check for differences?
> > Somewhere on the pixel path (between the display controller and the
> > DSI) there is a block that inverts the polarity. I can't remember
> > exactly what was the role of this block, but the polarity is inverted
> > when eLCDIF is used in combination with the DSI.
> > If you take a look at my DSI driver from NXP releases (I guess you have
> > them), you will see there is a hack in mode_fixup:
> >
> > unsigned int *flags = &mode->flags;
> > if (dsi->sync_pol {
> > *flags |= DRM_MODE_FLAG_PHSYNC;
> > *flags |= DRM_MODE_FLAG_PVSYNC;
> > *flags &= ~DRM_MODE_FLAG_NHSYNC;
> > *flags &= ~DRM_MODE_FLAG_NVSYNC;
> > } else {
> > *flags &= ~DRM_MODE_FLAG_PHSYNC;
> > *flags &= ~DRM_MODE_FLAG_PVSYNC;
> > *flags |= DRM_MODE_FLAG_NHSYNC;
> > *flags |= DRM_MODE_FLAG_NVSYNC;
> > }
>
> Thanks for the suggestion! I'll try that.
>
> >
> > I know it's not clean, but it works for now. You can try this in your
> > driver and see if it helps.
> > These days I will also take your nwl-dsi driver and test it, and also
> > add support for bridge and eLCDIF to see if I can make it work.
>
> I have hacky bridge support over here already. Give me some days to
> clean it up and it might safe you some work.
Your suggestion above (plus some other fixes) worked and
mxsfb+nwl+mixel-dphy works over here. I'll try to send a v1 of the nwl
driver out during the week.
Cheers,
-- Guido
> Cheers,
> -- Guido
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [RFC,v3 8/9] media: platform: Add Mediatek ISP P1 SCP communication
From: Jungo Lin @ 2019-07-21 2:18 UTC (permalink / raw)
To: Tomasz Figa
Cc: devicetree, sean.cheng, frederic.chen, rynn.wu, srv_heupstream,
robh, ryan.yu, frankie.chiu, hverkuil, ddavenport, sj.huang,
linux-mediatek, laurent.pinchart, matthias.bgg, mchehab,
linux-arm-kernel, linux-media
In-Reply-To: <20190710095827.GC181405@chromium.org>
Hi Tomasz:
On Wed, 2019-07-10 at 18:58 +0900, Tomasz Figa wrote:
> Hi Jungo,
>
> On Tue, Jun 11, 2019 at 11:53:43AM +0800, Jungo Lin wrote:
> > This patch adds communication with the co-processor on the SoC
> > through the SCP driver. It supports bi-directional commands
> > to exchange data and perform command flow control function.
> >
> > Signed-off-by: Jungo Lin <jungo.lin@mediatek.com>
> > ---
> > This patch depends on "Add support for mt8183 SCP"[1].
> >
> > [1] https://patchwork.kernel.org/cover/10972143/
> > ---
> > .../platform/mtk-isp/isp_50/cam/Makefile | 1 +
> > .../platform/mtk-isp/isp_50/cam/mtk_cam-scp.c | 371 ++++++++++++++++++
> > .../platform/mtk-isp/isp_50/cam/mtk_cam-scp.h | 207 ++++++++++
> > 3 files changed, 579 insertions(+)
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-scp.c
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-scp.h
> >
>
> Thanks for the patch! Please see my comments inline.
>
> [snip]
>
Thank you for your comments. Please check my replies inline.
[snip]
> > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-scp.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-scp.c
> > new file mode 100644
> > index 000000000000..04519d0b942f
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-scp.c
> > @@ -0,0 +1,371 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/kthread.h>
> > +#include <linux/platform_data/mtk_scp.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/remoteproc.h>
> > +#include <linux/sched.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +#include <linux/vmalloc.h>
> > +
> > +#include "mtk_cam.h"
> > +
> > +static void isp_composer_deinit(struct mtk_isp_p1_ctx *isp_ctx)
> > +{
> > + struct mtk_isp_queue_work *ipi_job, *tmp_ipi_job;
> > + struct isp_p1_device *p1_dev = p1_ctx_to_dev(isp_ctx);
> > +
> > + atomic_set(&isp_ctx->cmd_queued, 0);
> > + atomic_set(&isp_ctx->composer_txlist.queue_cnt, 0);
> > + atomic_set(&isp_ctx->composing_frame, 0);
> > + atomic_set(&isp_ctx->ipi_occupied, 0);
>
> Is there any point to set them if we are deinitalizing? Moreover,
> isp_composer_init() would set them once we start again.
>
We will remove these variable assignments.
> > +
> > + spin_lock(&isp_ctx->composer_txlist.lock);
> > + list_for_each_entry_safe(ipi_job, tmp_ipi_job,
> > + &isp_ctx->composer_txlist.queue,
> > + list_entry) {
> > + list_del(&ipi_job->list_entry);
> > + kfree(ipi_job);
> > + }
> > + atomic_set(&isp_ctx->composer_txlist.queue_cnt, 0);
> > + spin_unlock(&isp_ctx->composer_txlist.lock);
> > +
> > + mutex_lock(&isp_ctx->lock);
> > + if (isp_ctx->composer_tx_thread.thread) {
> > + kthread_stop(isp_ctx->composer_tx_thread.thread);
>
> Shouldn't the thread be stopped at this point already? If not, wouldn't the
> atomic_set() at the beginning of this function confuse it?
>
> In any case, this should be greatly simplified after we move to a workqueue,
> with one work per one task to do, as per other comments.
>
We will simplify the IPI sending mechanism and remove these kthread
handling.
> > + wake_up_interruptible(&isp_ctx->composer_tx_thread.wq);
> > + isp_ctx->composer_tx_thread.thread = NULL;
> > + }
> > +
> > + if (isp_ctx->composer_deinit_thread.thread) {
> > + wake_up(&isp_ctx->composer_deinit_thread.wq);
> > + isp_ctx->composer_deinit_thread.thread = NULL;
> > + }
> > + mutex_unlock(&isp_ctx->lock);
> > +
> > + pm_runtime_put_sync(&p1_dev->pdev->dev);
>
> No need to use the sync variant.
>
We don't get this point. If we will call pm_runtime_get_sync in
mtk_isp_hw_init function, will we need to call
pm_runtime_put_sync_autosuspend in mtk_isp_hw_release in next patch?
As we know, we should call runtime pm functions in pair.
> > +}
> > +
> > +/*
> > + * Two kinds of flow control in isp_composer_tx_work.
> > + *
> > + * Case 1: IPI commands flow control. The maximum number of command queues is 3.
> > + * There are two types of IPI commands (SCP_ISP_CMD/SCP_ISP_FRAME) in P1 driver.
> > + * It is controlled by ipi_occupied.
>
> ISP_COMPOSING_MAX_NUM is defined to 4, not 3. Is that expected?
>
In this version, we use async. scp_ipi_send function call with wait = 0.
If kernel sends too many P1 IPI commands in short time, P1 task in SCP
may miss some IPI command due to the IPI command processing time and the
size of command queue in SCP side. In order to avoid this kind of
condition, we use ISP_COMPOSING_MAX_NUM to control the sending flow of
IPI command in kernel side. The ISP_COMPOSING_MAX_NUM is changed to 4
for Chromium EC OS. We just miss to update the comment here.
In new version, we will change to use sync. scp_ipi_send function call
with non-zero wait variable. Based on this, we could remove IPI command
flow control in P1 driver.
> > + * The priority of SCP_ISP_CMD is higher than SCP_ISP_FRAME.
>
> What does it mean and why is it so?
>
In the origin design, SCP_ISP_CMD & SCP_ISP_FRAME are sending in the
same command queue by order. However, if we receive ISP_CMD_DEINIT
command, we will like to send this command firstly to SCP before
SCP_ISP_FRAME are queued in the queue. So we need to have one command
prioritize design here. Btw, in the new design, SCP_ISP_CMD &
SCP_ISP_FRAME are sent independent and we can remove this.
> > + *
> > + * Case 2: Frame buffers flow control. The maximum number of frame buffers is 3.
> > + * It is controlled by composing_frame.
> > + * Frame buffer is sent by SCP_ISP_FRAME command.
>
> Case 1 already mentions SCP_ISP_FRAME. What's the difference between that
> and case 2?
>
For case 2, it is related to frame request handling with CQ buffer.
We send frame request data via SCP_ISP_FRAME to compose CQ buffers in
SCP. The maximum CQ buffers in SCP are 3. So in kernel side, we can't
send any SCP_ISP_FRAME command to SCP when the CQ buffers are full in
SCP until ISP HW has output the new frame with the corresponding CQ
buffer.
In the new design, this will be controlled by mtk_cam_dev_req_try_queue
function with MTK_ISP_MAX_RUNNING_JOBS.
void mtk_cam_dev_req_try_queue(struct mtk_cam_dev *cam)
{
struct mtk_cam_dev_request *req, *req_prev;
struct list_head enqueue_job_list;
int buffer_cnt = atomic_read(&cam->running_job_count);
unsigned long flags;
if (!cam->streaming || buffer_cnt >= MTK_ISP_MAX_RUNNING_JOBS) {
dev_dbg(cam->dev, "stream off or buffers are full:%d\n",
buffer_cnt);
return;
}
INIT_LIST_HEAD(&enqueue_job_list);
spin_lock(&cam->pending_job_lock);
list_for_each_entry_safe(req, req_prev, &cam->pending_job_list, list) {
list_del(&req->list);
list_add_tail(&req->list, &enqueue_job_list);
if (atomic_inc_return(&cam->running_job_count) >=
MTK_ISP_MAX_RUNNING_JOBS)
break;
}
spin_unlock(&cam->pending_job_lock);
list_for_each_entry_safe(req, req_prev, &enqueue_job_list, list) {
list_del(&req->list);
spin_lock_irqsave(&cam->running_job_lock, flags);
list_add_tail(&req->list, &cam->running_job_list);
spin_unlock_irqrestore(&cam->running_job_lock, flags);
mtk_isp_req_enqueue(cam, req);
}
}
> > + */
> > +static int isp_composer_tx_work(void *data)
> > +{
> > + struct mtk_isp_p1_ctx *isp_ctx = (struct mtk_isp_p1_ctx *)data;
> > + struct isp_p1_device *p1_dev = p1_ctx_to_dev(isp_ctx);
> > + struct device *dev = &p1_dev->pdev->dev;
> > + struct mtk_isp_queue_work *isp_composer_work, *tmp_ipi_job;
> > + struct isp_queue *composer_txlist = &isp_ctx->composer_txlist;
> > + int ret;
> > +
> > + while (1) {
> > + ret = wait_event_interruptible
> > + (isp_ctx->composer_tx_thread.wq,
> > + (atomic_read(&composer_txlist->queue_cnt) > 0 &&
> > + atomic_read(&isp_ctx->ipi_occupied)
> > + < ISP_COMPOSING_MAX_NUM &&
> > + atomic_read(&isp_ctx->composing_frame)
> > + < ISP_FRAME_COMPOSING_MAX_NUM) ||
> > + (atomic_read(&isp_ctx->cmd_queued) > 0 &&
> > + atomic_read(&isp_ctx->ipi_occupied)
> > + < ISP_COMPOSING_MAX_NUM) ||
> > + kthread_should_stop());
> > +
> > + if (kthread_should_stop())
> > + break;
> > +
> > + spin_lock(&composer_txlist->lock);
> > + if (atomic_read(&isp_ctx->cmd_queued) > 0) {
> > + list_for_each_entry_safe(isp_composer_work, tmp_ipi_job,
> > + &composer_txlist->queue,
> > + list_entry) {
> > + if (isp_composer_work->type == SCP_ISP_CMD) {
> > + dev_dbg(dev, "Found a cmd\n");
> > + break;
> > + }
> > + }
> > + } else {
> > + if (atomic_read(&isp_ctx->composing_frame) >=
> > + ISP_FRAME_COMPOSING_MAX_NUM) {
> > + spin_unlock(&composer_txlist->lock);
> > + continue;
> > + }
> > + isp_composer_work =
> > + list_first_entry_or_null
> > + (&composer_txlist->queue,
> > + struct mtk_isp_queue_work,
> > + list_entry);
> > + }
>
> I don't understand why this special handling of CMD vs FRAME is here, so I
> might be missing something, but would we really lose anything if we just
> simply removed it and queued everything in order?
>
> Moreover, in V4L2, buffer queue and control operations are serialized wrt
> each other, so we probably wouldn't even have a chance to hit a case when we
> need to prioritize a CMD IPI over a FRAME IPI.
>
Yes, this implementation is complicated and we will remove
implementation in next patch. We will simplify current implementation by
using:
1. Use sync. scp_ipi_send function call
2. Use workqueue for SCP_ISP_FRAME sending
> > +
> > + list_del(&isp_composer_work->list_entry);
> > + atomic_dec(&composer_txlist->queue_cnt);
> > + spin_unlock(&composer_txlist->lock);
> > +
> > + if (isp_composer_work->type == SCP_ISP_CMD) {
> > + scp_ipi_send
> > + (p1_dev->scp_pdev,
> > + SCP_IPI_ISP_CMD,
> > + &isp_composer_work->cmd,
> > + sizeof(isp_composer_work->cmd),
> > + 0);
> > + atomic_dec(&isp_ctx->cmd_queued);
> > + atomic_inc(&isp_ctx->ipi_occupied);
> > + dev_dbg(dev,
> > + "%s cmd id %d sent, %d ipi buf occupied",
> > + __func__,
> > + isp_composer_work->cmd.cmd_id,
> > + atomic_read(&isp_ctx->ipi_occupied));
> > + } else if (isp_composer_work->type == SCP_ISP_FRAME) {
> > + scp_ipi_send
> > + (p1_dev->scp_pdev,
> > + SCP_IPI_ISP_FRAME,
> > + &isp_composer_work->frameparams,
> > + sizeof(isp_composer_work->frameparams),
> > + 0);
> > + atomic_inc(&isp_ctx->ipi_occupied);
> > + atomic_inc(&isp_ctx->composing_frame);
>
> Why do we need composing frame here, if ipi_occupied already limits us to 3?
>
If we send SCP_ISP_FRAME command, we need to increase ipi_occupied with
1 for IPI command sending command flow and increase composing_frame with
1 for CQ buffers composing. But this implementation will be removed.
> > + dev_dbg(dev,
> > + "%s frame %d sent, %d ipi, %d CQ bufs occupied",
> > + __func__,
> > + isp_composer_work->frameparams.frame_seq_no,
> > + atomic_read(&isp_ctx->ipi_occupied),
> > + atomic_read(&isp_ctx->composing_frame));
> > + } else {
> > + dev_err(dev,
> > + "ignore IPI type: %d!\n",
> > + isp_composer_work->type);
> > + }
> > + kfree(isp_composer_work);
> > + }
> > + return ret;
> > +}
>
> The function above is way too complicated than it should be. I'd suggest a
> model similar to what we ended up in the DIP driver:
> > - a freezable workqueue created for ISP composing works,
> > - each ISP composing work entry would have a struct work_struct embedded,
> > - isp_composer_enqueue() would enqueue the work_struct to the workqueue
> > above,
> > - the workqueue would keep a queue of works itself, so driver's own list
> > wouldn't be needed anymore,
> > - similarly, each execution of the work func would operate on its own ISP
> > composing work, so things like checking for list emptiness, waiting for
> > work to be queued, etc. wouldn't be needed,
> > - freezability of the workqueue would ensure nice synchonization with
> > system suspend/resume (although one would still need to wait for the
> > hardware/firmware to complete).
>
> WDYT?
>
yes, we will adopt your suggestion to re-factor current implementation.
Below is new implementation.
void mtk_isp_req_enqueue(struct mtk_cam_dev *cam,
struct mtk_cam_dev_request *req)
{
struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev);
int ret;
/* Accumulated frame sequence number */
req->frame_params.frame_seq_no = ++p1_dev->enqueue_frame_seq_no;
INIT_WORK(&req->frame_work, isp_tx_frame_worker);
ret = queue_work(p1_dev->composer_wq, &req->frame_work);
if (!ret)
dev_dbg(cam->dev, "frame_no:%d queue_work failed\n",
req->frame_params.frame_seq_no, ret);
else
dev_dbg(cam->dev, "Enqueue fd:%s frame_seq_no:%d job cnt:%d\n",
req->req.debug_str, req->frame_params.frame_seq_no,
atomic_read(&cam->running_job_count));
}
static void isp_tx_frame_worker(struct work_struct *work)
{
struct mtk_cam_dev_request *req =
container_of(work, struct mtk_cam_dev_request, frame_work);
struct mtk_cam_dev *cam =
container_of(req->req.mdev, struct mtk_cam_dev, media_dev);
struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev);
scp_ipi_send(p1_dev->scp_pdev, SCP_IPI_ISP_FRAME, &req->frame_params,
sizeof(req->frame_params), MTK_ISP_IPI_SEND_TIMEOUT);
}
> > +
> > +static int isp_composer_deinit_work(void *data)
> > +{
> > + struct mtk_isp_p1_ctx *isp_ctx = (struct mtk_isp_p1_ctx *)data;
> > + struct isp_p1_device *p1_dev = p1_ctx_to_dev(data);
> > + struct device *dev = &p1_dev->pdev->dev;
> > +
> > + wait_event_interruptible(isp_ctx->composer_deinit_thread.wq,
> > + atomic_read(&isp_ctx->scp_state) == SCP_OFF ||
> > + kthread_should_stop());
> > +
> > + dev_dbg(dev, "%s run deinit", __func__);
> > + isp_composer_deinit(isp_ctx);
> > +
> > + return 0;
> > +}
> > +
> > +static void isp_composer_handler(void *data, unsigned int len, void *priv)
> > +{
> > + struct mtk_isp_p1_ctx *isp_ctx = (struct mtk_isp_p1_ctx *)priv;
> > + struct isp_p1_device *p1_dev = p1_ctx_to_dev(isp_ctx);
> > + struct device *dev = &p1_dev->pdev->dev;
> > + struct mtk_isp_scp_p1_cmd *ipi_msg;
> > +
> > + ipi_msg = (struct mtk_isp_scp_p1_cmd *)data;
>
> Should we check that len == sizeof(*ipi_msg)? (Or at least >=, if data could
> contain some extra bytes at the end.)
>
The len parameter is the actual sending bytes from SCP to kernel.
In the runtime, it is only 6 bytes for isp_ack_info command
However, sizeof(*ipi_msg) is large due to struct mtk_isp_scp_p1_cmd is
union structure.
> > +
> > + if (ipi_msg->cmd_id != ISP_CMD_ACK)
> > + return;
> > +
> > + if (ipi_msg->ack_info.cmd_id == ISP_CMD_FRAME_ACK) {
> > + dev_dbg(dev, "ack frame_num:%d",
> > + ipi_msg->ack_info.frame_seq_no);
> > + atomic_set(&isp_ctx->composed_frame_id,
> > + ipi_msg->ack_info.frame_seq_no);
>
> I suppose we are expecting here that ipi_msg->ack_info.frame_seq_no would be
> just isp_ctx->composed_frame_id + 1, right? If not, we probably dropped some
> frames and we should handle that somehow.
>
No, we use isp_ctx->composed_frame_id to save which frame sequence
number are composed done in SCP. In new design, we will move this from
isp_ctx to p1_dev.
if (ipi_msg->ack_info.cmd_id == ISP_CMD_FRAME_ACK) {
atomic_set(&p1_dev->composed_frame_seq_no,
ipi_msg->ack_info.frame_seq_no);
dev_dbg(p1_dev->dev, "ack frame_num:%d\n",
p1_dev->composed_frame_seq_no);
}
> > + } else if (ipi_msg->ack_info.cmd_id == ISP_CMD_DEINIT) {
> > + dev_dbg(dev, "ISP_CMD_DEINIT is acked");
> > + atomic_set(&isp_ctx->scp_state, SCP_OFF);
> > + wake_up_interruptible(&isp_ctx->composer_deinit_thread.wq);
> > + }
> > +
> > + atomic_dec_return(&isp_ctx->ipi_occupied);
> > + wake_up_interruptible(&isp_ctx->composer_tx_thread.wq);
> > +}
> > +
> > +int isp_composer_init(struct device *dev)
> > +{
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + int ret;
> > +
> > + ret = scp_ipi_register(p1_dev->scp_pdev,
> > + SCP_IPI_ISP_CMD,
> > + isp_composer_handler,
> > + isp_ctx);
> > + if (ret)
> > + return ret;
> > +
> > + atomic_set(&isp_ctx->cmd_queued, 0);
> > + atomic_set(&isp_ctx->composer_txlist.queue_cnt, 0);
> > + atomic_set(&isp_ctx->composing_frame, 0);
> > + atomic_set(&isp_ctx->ipi_occupied, 0);
> > + atomic_set(&isp_ctx->scp_state, SCP_ON);
> > +
> > + mutex_lock(&isp_ctx->lock);
> > + if (!isp_ctx->composer_tx_thread.thread) {
> > + init_waitqueue_head(&isp_ctx->composer_tx_thread.wq);
> > + INIT_LIST_HEAD(&isp_ctx->composer_txlist.queue);
> > + spin_lock_init(&isp_ctx->composer_txlist.lock);
> > + isp_ctx->composer_tx_thread.thread =
> > + kthread_run(isp_composer_tx_work, isp_ctx,
> > + "isp_composer_tx");
> > + if (IS_ERR(isp_ctx->composer_tx_thread.thread)) {
> > + dev_err(dev, "unable to start kthread\n");
> > + isp_ctx->composer_tx_thread.thread = NULL;
> > + goto nomem;
>
> Why nomem?
>
It is wrong. Need to correct with
ERR_PTR(isp_ctx->composer_tx_thread.thread).
These kthread handling will be removed in next patch.
> > + }
> > + } else {
> > + dev_warn(dev, "old tx thread is existed\n");
>
> This shouldn't be possible to happen.
>
Yes, it should not be happen. Otherwise, there is a bug.
> > + }
> > +
> > + if (!isp_ctx->composer_deinit_thread.thread) {
> > + init_waitqueue_head(&isp_ctx->composer_deinit_thread.wq);
> > + isp_ctx->composer_deinit_thread.thread =
> > + kthread_run(isp_composer_deinit_work, isp_ctx,
> > + "isp_composer_deinit_work");
>
> Why do we need to deinit from another kthread?
>
This code will be removed in next patch.
> > + if (IS_ERR(isp_ctx->composer_deinit_thread.thread)) {
> > + dev_err(dev, "unable to start kthread\n");
> > + isp_ctx->composer_deinit_thread.thread = NULL;
> > + goto nomem;
> > + }
> > + } else {
> > + dev_warn(dev, "old rx thread is existed\n");
>
> rx? The code above seems to refer to deinit.
>
Got it.
> > + }
> > + mutex_unlock(&isp_ctx->lock);
> > +
> > + return 0;
> > +
> > +nomem:
> > + mutex_unlock(&isp_ctx->lock);
> > +
> > + return -ENOMEM;
>
> We should return the original error code here.
>
Got it.
> > +}
> > +
> > +void isp_composer_enqueue(struct device *dev,
> > + void *data,
> > + enum mtk_isp_scp_type type)
> > +{
> > + struct mtk_isp_queue_work *isp_composer_work;
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
>
> Just pass p1_dev to this function instead of dev.
>
Fix in next patch.
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +
> > + isp_composer_work = kzalloc(sizeof(*isp_composer_work), GFP_KERNEL);
>
> For most of the cases, it should be possible to preallocate this, e.g.
> > - for FRAME, this could be inside the request struct,
> > - for buffer queue it could be inside the buffer struct.
>
> I'd suggest making the caller responsible for allocating if needed.
>
Fix in next patch.
> > + isp_composer_work->type = type;
> > +
> > + switch (type) {
> > + case SCP_ISP_CMD:
> > + memcpy(&isp_composer_work->cmd, data,
> > + sizeof(isp_composer_work->cmd));
> > + dev_dbg(dev, "Enq ipi cmd id:%d\n",
> > + isp_composer_work->cmd.cmd_id);
> > +
> > + spin_lock(&isp_ctx->composer_txlist.lock);
> > + list_add_tail(&isp_composer_work->list_entry,
> > + &isp_ctx->composer_txlist.queue);
> > + atomic_inc(&isp_ctx->composer_txlist.queue_cnt);
> > + spin_unlock(&isp_ctx->composer_txlist.lock);
> > +
> > + atomic_inc(&isp_ctx->cmd_queued);
> > + wake_up_interruptible(&isp_ctx->composer_tx_thread.wq);
> > + break;
> > + case SCP_ISP_FRAME:
> > + memcpy(&isp_composer_work->frameparams, data,
> > + sizeof(isp_composer_work->frameparams));
> > + dev_dbg(dev, "Enq ipi frame_num:%d\n",
> > + isp_composer_work->frameparams.frame_seq_no);
> > +
> > + spin_lock(&isp_ctx->composer_txlist.lock);
> > + list_add_tail(&isp_composer_work->list_entry,
> > + &isp_ctx->composer_txlist.queue);
> > + atomic_inc(&isp_ctx->composer_txlist.queue_cnt);
> > + spin_unlock(&isp_ctx->composer_txlist.lock);
> > +
> > + wake_up_interruptible(&isp_ctx->composer_tx_thread.wq);
>
> The code in both cases is almost exactly the same. The only difference is
> the memcpy destination and size and whether isp_ctx->cmd_queued is
> incremented or not.
>
> The memcpy will go away if my comment above is addressed and so that would
> go down to making the cmd_queued increment conditional.
>
This function will be removed in next patch.
We will call scp_ipi_send directly in the caller, such as:
void mtk_isp_hw_config(struct mtk_cam_dev *cam,
struct p1_config_param *config_param)
{
struct mtk_isp_scp_p1_cmd composer_tx_cmd;
struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev);
memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd));
composer_tx_cmd.cmd_id = ISP_CMD_CONFIG;
memcpy(&composer_tx_cmd.config_param, config_param,
sizeof(*config_param));
scp_ipi_send(p1_dev->scp_pdev, SCP_IPI_ISP_CMD, &composer_tx_cmd,
sizeof(composer_tx_cmd), MTK_ISP_IPI_SEND_TIMEOUT);
}
void mtk_isp_stream(struct mtk_cam_dev *cam, int on)
{
struct mtk_isp_scp_p1_cmd composer_tx_cmd;
struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev);
memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd));
composer_tx_cmd.cmd_id = ISP_CMD_STREAM;
composer_tx_cmd.is_stream_on = on;
scp_ipi_send(p1_dev->scp_pdev, SCP_IPI_ISP_CMD, &composer_tx_cmd,
sizeof(composer_tx_cmd), MTK_ISP_IPI_SEND_TIMEOUT);
}
static void isp_composer_hw_deinit(struct mtk_isp_p1_device *p1_dev)
{
struct mtk_isp_scp_p1_cmd composer_tx_cmd;
memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd));
composer_tx_cmd.cmd_id = ISP_CMD_DEINIT;
scp_ipi_send(p1_dev->scp_pdev, SCP_IPI_ISP_CMD, &composer_tx_cmd,
sizeof(composer_tx_cmd), MTK_ISP_IPI_SEND_TIMEOUT);
isp_composer_uninit(p1_dev);
}
> > + break;
> > + default:
> > + break;
> > + }
> > +}
> > +
> > +void isp_composer_hw_init(struct device *dev)
> > +{
> > + struct mtk_isp_scp_p1_cmd composer_tx_cmd;
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +
> > + memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd));
> > + composer_tx_cmd.cmd_id = ISP_CMD_INIT;
> > + composer_tx_cmd.frameparam.hw_module = isp_ctx->isp_hw_module;
> > + composer_tx_cmd.frameparam.cq_addr.iova = isp_ctx->scp_mem_iova;
> > + composer_tx_cmd.frameparam.cq_addr.scp_addr = isp_ctx->scp_mem_pa;
>
> Should we also specify the size of the buffer? Otherwise we could end up
> with some undetectable overruns.
>
The size of SCP composer's memory is fixed to 0x200000.
Is it necessary to specify the size of this buffer?
#define MTK_ISP_COMPOSER_MEM_SIZE 0x200000
ptr = dma_alloc_coherent(p1_dev->cam_dev.smem_dev,
MTK_ISP_COMPOSER_MEM_SIZE, &addr, GFP_KERNEL);
> > + isp_composer_enqueue(dev, &composer_tx_cmd, SCP_ISP_CMD);
> > +}
> > +
> > +void isp_composer_meta_config(struct device *dev,
> > + unsigned int dma)
> > +{
> > + struct mtk_isp_scp_p1_cmd composer_tx_cmd;
> > +
> > + memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd));
> > + composer_tx_cmd.cmd_id = ISP_CMD_CONFIG_META;
> > + composer_tx_cmd.cfg_meta_out_param.enabled_meta_dmas = dma;
> > + isp_composer_enqueue(dev, &composer_tx_cmd, SCP_ISP_CMD);
> > +}
> > +
> > +void isp_composer_hw_config(struct device *dev,
> > + struct p1_config_param *config_param)
> > +{
> > + struct mtk_isp_scp_p1_cmd composer_tx_cmd;
> > +
> > + memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd));
> > + composer_tx_cmd.cmd_id = ISP_CMD_CONFIG;
> > + memcpy(&composer_tx_cmd.config_param, config_param,
> > + sizeof(*config_param));
> > + isp_composer_enqueue(dev, &composer_tx_cmd, SCP_ISP_CMD);
> > +}
> > +
> > +void isp_composer_stream(struct device *dev, int on)
> > +{
> > + struct mtk_isp_scp_p1_cmd composer_tx_cmd;
> > +
> > + memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd));
> > + composer_tx_cmd.cmd_id = ISP_CMD_STREAM;
> > + composer_tx_cmd.is_stream_on = on;
> > + isp_composer_enqueue(dev, &composer_tx_cmd, SCP_ISP_CMD);
> > +}
> > +
> > +void isp_composer_hw_deinit(struct device *dev)
> > +{
> > + struct mtk_isp_scp_p1_cmd composer_tx_cmd;
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + int ret;
> > +
> > + memset(&composer_tx_cmd, 0, sizeof(composer_tx_cmd));
> > + composer_tx_cmd.cmd_id = ISP_CMD_DEINIT;
> > + isp_composer_enqueue(dev, &composer_tx_cmd, SCP_ISP_CMD);
> > +
> > + /* Wait for ISP_CMD_DEINIT command is handled done */
> > + ret = wait_event_timeout(isp_ctx->composer_deinit_thread.wq,
> > + atomic_read(&isp_ctx->scp_state) == SCP_OFF,
> > + msecs_to_jiffies(2000));
> > + if (ret)
> > + return;
> > +
> > + dev_warn(dev, "Timeout & local de-init\n");
> > + isp_composer_deinit(isp_ctx);
> > +}
> > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-scp.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-scp.h
> > new file mode 100644
> > index 000000000000..fbd8593e9c2d
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-scp.h
> > @@ -0,0 +1,207 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + */
> > +
> > +#ifndef _MTK_ISP_SCP_H
> > +#define _MTK_ISP_SCP_H
> > +
> > +#include <linux/types.h>
> > +
> > +#include "mtk_cam-v4l2-util.h"
> > +
> > +/*
> > + * struct img_size - image size information.
> > + *
> > + * @w: image width, the unit is pixel
> > + * @h: image height, the unit is pixel
> > + * @xsize: bytes per line based on width.
> > + * @stride: bytes per line when changing line.
> > + * Normally, calculate new STRIDE based on
> > + * xsize + HW constrain(page or align).
> > + *
> > + */
> > +struct img_size {
> > + __u32 w;
> > + __u32 h;
> > + __u32 xsize;
> > + __u32 stride;
> > +} __packed;
> > +
> > +/*
> > + * struct img_buffer - buffer address information.
> > + *
> > + * @iova: DMA address for external devices.
> > + * @scp_addr: SCP address for external co-process unit.
> > + *
> > + */
> > +struct img_buffer {
> > + __u32 iova;
> > + __u32 scp_addr;
> > +} __packed;
> > +
> > +struct p1_img_crop {
> > + __u32 left;
> > + __u32 top;
> > + __u32 width;
> > + __u32 height;
> > +} __packed;
> > +
> > +struct p1_img_output {
> > + struct img_buffer buffer;
> > + struct img_size size;
> > + struct p1_img_crop crop;
> > + __u8 pixel_byte;
> > + __u32 img_fmt;
> > +} __packed;
>
> Please document.
>
Fix in next patch.
> > +
> > +/*
> > + * struct cfg_in_param - image input parameters structure.
> > + * Normally, it comes from sensor information.
> > + *
> > + * @continuous: indicate the sensor mode.
> > + * 1: continuous
> > + * 0: single
> > + * @subsample: indicate to enables SOF subsample or not.
> > + * @pixel_mode: describe 1/2/4 pixels per clock cycle.
> > + * @data_pattern: describe input data pattern.
> > + * @raw_pixel_id: bayer sequence.
> > + * @tg_fps: the fps rate of TG (time generator).
> > + * @img_fmt: the image format of input source.
> > + * @p1_img_crop: the crop configuration of input source.
> > + *
> > + */
> > +struct cfg_in_param {
> > + __u8 continuous;
> > + __u8 subsample;
> > + __u8 pixel_mode;
> > + __u8 data_pattern;
> > + __u8 raw_pixel_id;
> > + __u16 tg_fps;
> > + __u32 img_fmt;
> > + struct p1_img_crop crop;
> > +} __packed;
> > +
> > +/*
> > + * struct cfg_main_out_param - the image output parameters of main stream.
> > + *
> > + * @bypass: indicate this device is enabled or disabled or not .
>
> Remove the space before the period.
>
Fix in next patch.
> > + * @pure_raw: indicate the image path control.
> > + * 1: pure raw
> > + * 0: processing raw
> > + * @pure_raw_pack: indicate the image is packed or not.
> > + * 1: packed mode
> > + * 0: unpacked mode
> > + * @p1_img_output: the output image information.
> > + *
> > + */
> > +struct cfg_main_out_param {
> > + /* Bypass main out parameters */
> > + __u8 bypass;
> > + /* Control HW image raw path */
> > + __u8 pure_raw;
> > + /* Control HW image pack function */
>
> No need for these inline comments.
>
Fix in next patch.
> > + __u8 pure_raw_pack;
> > + struct p1_img_output output;
> > +} __packed;
> > +
> > +/*
> > + * struct cfg_resize_out_param - the image output parameters of
> > + * packed out stream.
> > + *
> > + * @bypass: indicate this device is enabled or disabled or not .
>
> Remove the space before the period.
>
Fix in next patch.
> > + * @p1_img_output: the output image information.
> > + *
> > + */
> > +struct cfg_resize_out_param {
> > + /* Bypass resize parameters */
>
> No need for this inline comment.
>
Fix in next patch.
> > + __u8 bypass;
> > + struct p1_img_output output;
> > +} __packed;
> > +
> > +/*
> > + * struct cfg_meta_out_param - output meta information.
> > + *
> > + * @enabled_meta_dmas: indicate which meta DMAs are enabled.
> > + *
> > + */
> > +struct cfg_meta_out_param {
> > + __u32 enabled_meta_dmas;
> > +} __packed;
> > +
> > +struct p1_config_param {
> > + /* Sensor/TG info */
> > + struct cfg_in_param cfg_in_param;
> > + /* IMGO DMA */
> > + struct cfg_main_out_param cfg_main_param;
> > + /* RRZO DMA */
> > + struct cfg_resize_out_param cfg_resize_param;
> > + /* 3A DMAs and other. */
> > + struct cfg_meta_out_param cfg_meta_param;
>
> Please change the inline comments to a kerneldoc comment at the top.
>
Fix in next patch.
> > +} __packed;
> > +
> > +struct p1_frame_param {
> > + /* frame sequence number */
> > + __u32 frame_seq_no;
> > + /* SOF index */
> > + __u32 sof_idx;
> > + /* The memory address of tuning buffer from user space */
>
> Ditto.
>
Fix in next patch.
> > + struct img_buffer dma_buffers[MTK_CAM_P1_TOTAL_NODES];
> > +} __packed;
> > +
> > +struct P1_meta_frame {
> > + __u32 enabled_dma;
> > + __u32 vb_index;
> > + struct img_buffer meta_addr;
> > +} __packed;
> > +
> > +struct isp_init_info {
> > + __u8 hw_module;
> > + struct img_buffer cq_addr;
> > +} __packed;
> > +
> > +struct isp_ack_info {
> > + __u8 cmd_id;
> > + __u32 frame_seq_no;
> > +} __packed;
> > +
> > +enum mtk_isp_scp_cmds {
> > + ISP_CMD_INIT,
> > + ISP_CMD_CONFIG,
> > + ISP_CMD_STREAM,
> > + ISP_CMD_DEINIT,
> > + ISP_CMD_ACK,
> > + ISP_CMD_FRAME_ACK,
> > + ISP_CMD_CONFIG_META,
> > + ISP_CMD_ENQUEUE_META,
> > + ISP_CMD_RESERVED,
> > +};
> > +
> > +struct mtk_isp_scp_p1_cmd {
> > + __u8 cmd_id;
> > + union {
> > + struct isp_init_info frameparam;
> > + struct p1_config_param config_param;
> > + struct cfg_meta_out_param cfg_meta_out_param;
> > + struct P1_meta_frame meta_frame;
> > + __u8 is_stream_on;
> > + struct isp_ack_info ack_info;
> > + };
> > +} __packed;
> > +
> > +enum mtk_isp_scp_type {
> > + SCP_ISP_CMD = 0,
> > + SCP_ISP_FRAME,
> > +};
>
> Please document all the structs and enum above using kerneldoc.
>
Fix in next patch.
> Best regards,
> Tomasz
>
Thank you for your valuable comments.
Best regards,
Jungo
^ permalink raw reply
* [GIT PULL] Devicetree fixes for 5.3-rc
From: Rob Herring @ 2019-07-21 2:37 UTC (permalink / raw)
To: Linus Torvalds; +Cc: devicetree, linux-kernel@vger.kernel.org, Frank Rowand
Linus,
Please pull a couple of DT binding fixes for 5.3-rc.
Rob
The following changes since commit abdfd52a295fb5731ab07b5c9013e2e39f4d1cbe:
Merge tag 'armsoc-defconfig' of
git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc (2019-07-19
17:27:27 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
tags/devicetree-fixes-for-5.3
for you to fetch changes up to e2297f7c3ab3b68dda2ac732b1767212019d3bdf:
dt-bindings: pinctrl: stm32: Fix missing 'clocks' property in
examples (2019-07-20 20:28:53 -0600)
----------------------------------------------------------------
Devicetree fixes for 5.3:
Fix several warnings/errors in validation of binding schemas.
----------------------------------------------------------------
Rob Herring (7):
dt-bindings: Ensure child nodes are of type 'object'
dt-bindings: riscv: Limit cpus schema to only check RiscV 'cpu' nodes
dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
dt-bindings: pinctrl: aspeed: Fix AST2500 example errors
dt-bindings: iio: avia-hx711: Fix avdd-supply typo in example
dt-bindings: iio: ad7124: Fix dtc warnings in example
dt-bindings: pinctrl: stm32: Fix missing 'clocks' property in examples
.../bindings/bus/allwinner,sun8i-a23-rsb.yaml | 1 +
.../devicetree/bindings/iio/adc/adi,ad7124.yaml | 71 +++++-----
.../devicetree/bindings/iio/adc/avia-hx711.yaml | 2 +-
.../bindings/mtd/allwinner,sun4i-a10-nand.yaml | 1 +
.../devicetree/bindings/mtd/nand-controller.yaml | 1 +
.../bindings/pinctrl/aspeed,ast2400-pinctrl.yaml | 4 +-
.../bindings/pinctrl/aspeed,ast2500-pinctrl.yaml | 9 +-
.../bindings/pinctrl/st,stm32-pinctrl.yaml | 7 +
Documentation/devicetree/bindings/riscv/cpus.yaml | 143 +++++++++------------
.../bindings/spi/allwinner,sun4i-a10-spi.yaml | 1 +
.../bindings/spi/allwinner,sun6i-a31-spi.yaml | 1 +
11 files changed, 119 insertions(+), 122 deletions(-)
^ permalink raw reply
* Re: [Question] orphan platform data header
From: Masahiro Yamada @ 2019-07-21 3:44 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Greg Kroah-Hartman, Linus Torvalds, Linux Kernel Mailing List,
linux-arm-kernel, DTML
In-Reply-To: <CAK8P3a2AeUpmNfFLJSvHT=AJ0kFRT2B=TWDm0HsTwoHt2jQ0gQ@mail.gmail.com>
Hi Arnd,
On Sat, Jul 20, 2019 at 10:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > masahiro@grover:~/ref/linux$ git grep netxbig_led_platform_data
> > drivers/leds/leds-netxbig.c: struct
> > netxbig_led_platform_data *pdata,
> > drivers/leds/leds-netxbig.c: struct
> > netxbig_led_platform_data *pdata)
> > drivers/leds/leds-netxbig.c: struct
> > netxbig_led_platform_data *pdata)
> > drivers/leds/leds-netxbig.c: struct netxbig_led_platform_data
> > *pdata = dev_get_platdata(&pdev->dev);
> > include/linux/platform_data/leds-kirkwood-netxbig.h:struct
> > netxbig_led_platform_data {
> >
> >
> >
> > So, what shall we do?
> >
> > Drop the board-file support? Or, keep it
> > in case somebody is still using their board-files
> > in downstream?
>
> Generally speaking, I'd remove the board file support in another
> case like this, but it's worth looking at when it was last used and by
> what.
>
> For this file, all boards got converted to DT, and the old setup
> code removed in commit ebc278f15759 ("ARM: mvebu: remove static
> LED setup for netxbig boards"), four years ago, so it's a fairly
> easy decision to make it DT only.
Thanks.
I see another case, which is difficult
to make a decision.
For example, drivers/spi/spi-tle62x0.c
This driver supports only board-file, but the board-file
is not found in upstream.
Unless I am terribly missing something,
there is no one who passes tle62x0_pdata
to this driver.
$ git grep tle62x0_pdata
drivers/spi/spi-tle62x0.c: struct tle62x0_pdata *pdata;
include/linux/spi/tle62x0.h:struct tle62x0_pdata {
But, removing board-file support
makes this driver completely useless...
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [Question] orphan platform data header
From: Arnd Bergmann @ 2019-07-21 9:09 UTC (permalink / raw)
To: Masahiro Yamada, Ben Dooks
Cc: Linux Kernel Mailing List, Linus Torvalds, Greg Kroah-Hartman,
DTML, linux-arm-kernel
In-Reply-To: <CAK7LNATPbCjwzVnAigsQ8tQRXjC31uxgPg3jgi7pwp+N1RPgWw@mail.gmail.com>
On Sun, Jul 21, 2019 at 5:45 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> On Sat, Jul 20, 2019 at 10:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > > So, what shall we do?
> > >
> > > Drop the board-file support? Or, keep it
> > > in case somebody is still using their board-files
> > > in downstream?
>>
> > For this file, all boards got converted to DT, and the old setup
> > code removed in commit ebc278f15759 ("ARM: mvebu: remove static
> > LED setup for netxbig boards"), four years ago, so it's a fairly
> > easy decision to make it DT only.
>
> I see another case, which is difficult
> to make a decision.
>
> For example, drivers/spi/spi-tle62x0.c
>
> This driver supports only board-file, but the board-file
> is not found in upstream.
>
> Unless I am terribly missing something,
> there is no one who passes tle62x0_pdata
> to this driver.
>
> $ git grep tle62x0_pdata
> drivers/spi/spi-tle62x0.c: struct tle62x0_pdata *pdata;
> include/linux/spi/tle62x0.h:struct tle62x0_pdata {
>
> But, removing board-file support
> makes this driver completely useless...
Adding Ben Dooks to Cc.
I suspect this driver is completely obsolete and should be removed.
For some reason, it's not an SPI controller driver like all the other
files in that directory, but implements low-level access to the state
of a particular SPI device.
However, there should not really be a low-level driver for it that
just exports the pins to user space. It should either be a gpiolib
driver to let other drivers talk to the pins, or a high-level driver that
exposes the intended functionality (watchdog, regulator, ...)
to those respective subsystems.
Arnd
^ permalink raw reply
* Re: [PATCH 01/10] drm/mxsfb: Update mxsfb to support a bridge
From: Guido Günther @ 2019-07-21 10:26 UTC (permalink / raw)
To: Robert Chiras
Cc: Marek Vasut, Mark Rutland, Pengutronix Kernel Team, devicetree,
David Airlie, linux-kernel, Rob Herring, dri-devel, Shawn Guo,
linux-arm-kernel, NXP Linux Team
In-Reply-To: <1561555938-21595-2-git-send-email-robert.chiras@nxp.com>
Hi Robert,
On Wed, Jun 26, 2019 at 04:32:09PM +0300, Robert Chiras wrote:
> Currently, the MXSFB DRM driver only supports a panel. But, its output
> display signal can also be redirected to another encoder, like a DSI
> controller. In this case, that DSI controller may act like a drm_bridge.
> In order support this use-case too, this patch adds support for
> drm_bridge in mxsfb.
>
> Signed-off-by: Robert Chiras <robert.chiras@nxp.com>
> ---
> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 46 +++++++++++++++++++++++++++++++++++---
> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 46 +++++++++++++++++++++++++++++++++-----
> drivers/gpu/drm/mxsfb/mxsfb_drv.h | 4 +++-
> drivers/gpu/drm/mxsfb/mxsfb_out.c | 26 +++++++++++----------
> drivers/gpu/drm/mxsfb/mxsfb_regs.h | 15 +++++++++++++
> 5 files changed, 116 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index 93f4133..14bde024 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> @@ -93,8 +93,11 @@ static void mxsfb_set_bus_fmt(struct mxsfb_drm_private *mxsfb)
>
> reg = readl(mxsfb->base + LCDC_CTRL);
>
> - if (mxsfb->connector.display_info.num_bus_formats)
> - bus_format = mxsfb->connector.display_info.bus_formats[0];
> + if (mxsfb->connector->display_info.num_bus_formats)
> + bus_format = mxsfb->connector->display_info.bus_formats[0];
> +
> + DRM_DEV_DEBUG_DRIVER(drm->dev, "Using bus_format: 0x%08X\n",
> + bus_format);
>
> reg &= ~CTRL_BUS_WIDTH_MASK;
> switch (bus_format) {
> @@ -122,6 +125,9 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
> clk_prepare_enable(mxsfb->clk_disp_axi);
> clk_prepare_enable(mxsfb->clk);
>
> + writel(CTRL2_OUTSTANDING_REQS__REQ_16,
> + mxsfb->base + LCDC_V4_CTRL2 + REG_SET);
> +
> /* If it was disabled, re-enable the mode again */
> writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET);
>
> @@ -131,12 +137,15 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
> writel(reg, mxsfb->base + LCDC_VDCTRL4);
>
> writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_SET);
> + writel(CTRL1_RECOVERY_ON_UNDERFLOW, mxsfb->base + LCDC_CTRL1 + REG_SET);
> }
>
> static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb)
> {
> u32 reg;
>
> + writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_CLR);
> +
> /*
> * Even if we disable the controller here, it will still continue
> * until its FIFOs are running out of data
> @@ -202,8 +211,9 @@ static dma_addr_t mxsfb_get_fb_paddr(struct mxsfb_drm_private *mxsfb)
>
> static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
> {
> + struct drm_device *drm = mxsfb->pipe.crtc.dev;
> struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode;
> - const u32 bus_flags = mxsfb->connector.display_info.bus_flags;
> + const u32 bus_flags = mxsfb->connector->display_info.bus_flags;
> u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
> int err;
>
> @@ -227,6 +237,13 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
>
> clk_set_rate(mxsfb->clk, m->crtc_clock * 1000);
>
> + DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock: %dkHz (actual: %dkHz)\n",
> + m->crtc_clock,
> + (int)(clk_get_rate(mxsfb->clk) / 1000));
> + DRM_DEV_DEBUG_DRIVER(drm->dev, "Connector bus_flags: 0x%08X\n",
> + bus_flags);
> + DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
> +
> writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) |
> TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay),
> mxsfb->base + mxsfb->devdata->transfer_count);
> @@ -279,6 +296,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb)
> dma_addr_t paddr;
>
> mxsfb_enable_axi_clk(mxsfb);
> + writel(0, mxsfb->base + LCDC_CTRL);
> mxsfb_crtc_mode_set_nofb(mxsfb);
>
> /* Write cur_buf as well to avoid an initial corrupt frame */
> @@ -302,6 +320,8 @@ void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
> {
> struct drm_simple_display_pipe *pipe = &mxsfb->pipe;
> struct drm_crtc *crtc = &pipe->crtc;
> + struct drm_framebuffer *fb = pipe->plane.state->fb;
> + struct drm_framebuffer *old_fb = old_state->fb;
> struct drm_pending_vblank_event *event;
> dma_addr_t paddr;
>
> @@ -324,4 +344,24 @@ void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
> writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
> mxsfb_disable_axi_clk(mxsfb);
> }
> +
> + if (!fb || !old_fb)
> + return;
> +
> + /*
> + * TODO: Currently, we only support pixel format change, but we need
> + * also to care about size changes too
> + */
> + if (old_fb->format->format != fb->format->format) {
> + struct drm_format_name_buf old_fmt_buf;
> + struct drm_format_name_buf new_fmt_buf;
> +
> + DRM_DEV_DEBUG_DRIVER(crtc->dev->dev,
> + "Switching pixel format: %s -> %s\n",
> + drm_get_format_name(old_fb->format->format,
> + &old_fmt_buf),
> + drm_get_format_name(fb->format->format,
> + &new_fmt_buf));
> + mxsfb_set_pixel_fmt(mxsfb, true);
This assumes mxsfb_set_pixel_fmt has two arguments which is being introduced
in the following commit. With that fixed:
Tested-by: Guido Günther <agx@sigxcpu.org>
Cheers,
-- Guido
> + }
> }
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index 6fafc90..0d171e9 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -98,9 +98,25 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe,
> struct drm_crtc_state *crtc_state,
> struct drm_plane_state *plane_state)
> {
> + struct drm_connector *connector;
> struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe);
> struct drm_device *drm = pipe->plane.dev;
>
> + if (!mxsfb->connector) {
> + list_for_each_entry(connector,
> + &drm->mode_config.connector_list,
> + head)
> + if (connector->encoder == &mxsfb->pipe.encoder) {
> + mxsfb->connector = connector;
> + break;
> + }
> + }
> +
> + if (!mxsfb->connector) {
> + dev_warn(drm->dev, "No connector attached, using default\n");
> + mxsfb->connector = &mxsfb->panel_connector;
> + }
> +
> pm_runtime_get_sync(drm->dev);
> drm_panel_prepare(mxsfb->panel);
> mxsfb_crtc_enable(mxsfb);
> @@ -126,6 +142,9 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe)
> drm_crtc_send_vblank_event(crtc, event);
> }
> spin_unlock_irq(&drm->event_lock);
> +
> + if (mxsfb->connector != &mxsfb->panel_connector)
> + mxsfb->connector = NULL;
> }
>
> static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe,
> @@ -223,16 +242,33 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags)
>
> ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs,
> mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL,
> - &mxsfb->connector);
> + mxsfb->connector);
> if (ret < 0) {
> dev_err(drm->dev, "Cannot setup simple display pipe\n");
> goto err_vblank;
> }
>
> - ret = drm_panel_attach(mxsfb->panel, &mxsfb->connector);
> - if (ret) {
> - dev_err(drm->dev, "Cannot connect panel\n");
> - goto err_vblank;
> + /*
> + * Attach panel only if there is one.
> + * If there is no panel attach, it must be a bridge. In this case, we
> + * need a reference to its connector for a proper initialization.
> + * We will do this check in pipe->enable(), since the connector won't
> + * be attached to an encoder until then.
> + */
> +
> + if (mxsfb->panel) {
> + ret = drm_panel_attach(mxsfb->panel, mxsfb->connector);
> + if (ret) {
> + dev_err(drm->dev, "Cannot connect panel\n");
> + goto err_vblank;
> + }
> + } else if (mxsfb->bridge) {
> + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe,
> + mxsfb->bridge);
> + if (ret) {
> + dev_err(drm->dev, "Cannot connect bridge\n");
> + goto err_vblank;
> + }
> }
>
> drm->mode_config.min_width = MXSFB_MIN_XRES;
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> index d975300..0b65b51 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> @@ -27,8 +27,10 @@ struct mxsfb_drm_private {
> struct clk *clk_disp_axi;
>
> struct drm_simple_display_pipe pipe;
> - struct drm_connector connector;
> + struct drm_connector panel_connector;
> + struct drm_connector *connector;
> struct drm_panel *panel;
> + struct drm_bridge *bridge;
> };
>
> int mxsfb_setup_crtc(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c b/drivers/gpu/drm/mxsfb/mxsfb_out.c
> index 91e76f9..b9acf2b 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_out.c
> @@ -22,7 +22,8 @@
> static struct mxsfb_drm_private *
> drm_connector_to_mxsfb_drm_private(struct drm_connector *connector)
> {
> - return container_of(connector, struct mxsfb_drm_private, connector);
> + return container_of(connector, struct mxsfb_drm_private,
> + panel_connector);
> }
>
> static int mxsfb_panel_get_modes(struct drm_connector *connector)
> @@ -77,22 +78,23 @@ static const struct drm_connector_funcs mxsfb_panel_connector_funcs = {
> int mxsfb_create_output(struct drm_device *drm)
> {
> struct mxsfb_drm_private *mxsfb = drm->dev_private;
> - struct drm_panel *panel;
> int ret;
>
> - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, NULL);
> + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0,
> + &mxsfb->panel, &mxsfb->bridge);
> if (ret)
> return ret;
>
> - mxsfb->connector.dpms = DRM_MODE_DPMS_OFF;
> - mxsfb->connector.polled = 0;
> - drm_connector_helper_add(&mxsfb->connector,
> - &mxsfb_panel_connector_helper_funcs);
> - ret = drm_connector_init(drm, &mxsfb->connector,
> - &mxsfb_panel_connector_funcs,
> - DRM_MODE_CONNECTOR_Unknown);
> - if (!ret)
> - mxsfb->panel = panel;
> + if (mxsfb->panel) {
> + mxsfb->connector = &mxsfb->panel_connector;
> + mxsfb->connector->dpms = DRM_MODE_DPMS_OFF;
> + mxsfb->connector->polled = 0;
> + drm_connector_helper_add(mxsfb->connector,
> + &mxsfb_panel_connector_helper_funcs);
> + ret = drm_connector_init(drm, mxsfb->connector,
> + &mxsfb_panel_connector_funcs,
> + DRM_MODE_CONNECTOR_Unknown);
> + }
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> index 932d7ea..71426aa 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> @@ -14,19 +14,31 @@
>
> #define LCDC_CTRL 0x00
> #define LCDC_CTRL1 0x10
> +#define LCDC_V4_CTRL2 0x20
> #define LCDC_V3_TRANSFER_COUNT 0x20
> #define LCDC_V4_TRANSFER_COUNT 0x30
> #define LCDC_V4_CUR_BUF 0x40
> #define LCDC_V4_NEXT_BUF 0x50
> #define LCDC_V3_CUR_BUF 0x30
> #define LCDC_V3_NEXT_BUF 0x40
> +#define LCDC_TIMING 0x60
> #define LCDC_VDCTRL0 0x70
> #define LCDC_VDCTRL1 0x80
> #define LCDC_VDCTRL2 0x90
> #define LCDC_VDCTRL3 0xa0
> #define LCDC_VDCTRL4 0xb0
> +#define LCDC_DVICTRL0 0xc0
> +#define LCDC_DVICTRL1 0xd0
> +#define LCDC_DVICTRL2 0xe0
> +#define LCDC_DVICTRL3 0xf0
> +#define LCDC_DVICTRL4 0x100
> +#define LCDC_V4_DATA 0x180
> +#define LCDC_V3_DATA 0x1b0
> #define LCDC_V4_DEBUG0 0x1d0
> #define LCDC_V3_DEBUG0 0x1f0
> +#define LCDC_AS_CTRL 0x210
> +#define LCDC_AS_BUF 0x220
> +#define LCDC_AS_NEXT_BUF 0x230
>
> #define CTRL_SFTRST (1 << 31)
> #define CTRL_CLKGATE (1 << 30)
> @@ -45,12 +57,15 @@
> #define CTRL_DF24 (1 << 1)
> #define CTRL_RUN (1 << 0)
>
> +#define CTRL1_RECOVERY_ON_UNDERFLOW (1 << 24)
> #define CTRL1_FIFO_CLEAR (1 << 21)
> #define CTRL1_SET_BYTE_PACKAGING(x) (((x) & 0xf) << 16)
> #define CTRL1_GET_BYTE_PACKAGING(x) (((x) >> 16) & 0xf)
> #define CTRL1_CUR_FRAME_DONE_IRQ_EN (1 << 13)
> #define CTRL1_CUR_FRAME_DONE_IRQ (1 << 9)
>
> +#define CTRL2_OUTSTANDING_REQS__REQ_16 (4 << 21)
> +
> #define TRANSFER_COUNT_SET_VCOUNT(x) (((x) & 0xffff) << 16)
> #define TRANSFER_COUNT_GET_VCOUNT(x) (((x) >> 16) & 0xffff)
> #define TRANSFER_COUNT_SET_HCOUNT(x) ((x) & 0xffff)
> --
> 2.7.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 02/10] drm/mxsfb: Update mxsfb with additional pixel formats
From: Guido Günther @ 2019-07-21 10:27 UTC (permalink / raw)
To: Robert Chiras
Cc: Marek Vasut, Stefan Agner, David Airlie, Daniel Vetter,
Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Fabio Estevam,
devicetree, linux-kernel, dri-devel, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel
In-Reply-To: <1561555938-21595-3-git-send-email-robert.chiras@nxp.com>
Hi,
I'm not very familiar with mxsfb, just some things that stood out while
looking at the code:
On Wed, Jun 26, 2019 at 04:32:10PM +0300, Robert Chiras wrote:
> Since version 4 of eLCDIF, there are some registers that can do
> transformations on the input data, like re-arranging the pixel
> components. By doing that, we can support more pixel formats.
> This patch adds support for X/ABGR and RGBX/A. Although, the local alpha
> is not supported by eLCDIF, the alpha pixel formats were added to the
> supported pixel formats but it will be ignored. This was necessary since
> there are systems (like Android) that requires such pixel formats.
>
> Also, add support for the following pixel formats:
> 16 bpp: RG16 ,BG16, XR15, XB15, AR15, AB15
> Set the bus format based on input from the user and panel capabilities.
> Save the bus format in crtc->mode.private_flags, so the bridge can use
> it.
>
> Signed-off-by: Robert Chiras <robert.chiras@nxp.com>
> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> ---
> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 158 ++++++++++++++++++++++++++++++-------
> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 30 +++++--
> drivers/gpu/drm/mxsfb/mxsfb_drv.h | 3 +-
> drivers/gpu/drm/mxsfb/mxsfb_regs.h | 100 ++++++++++++++---------
> 4 files changed, 217 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index 14bde024..e48396d 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> @@ -41,14 +41,17 @@ static u32 set_hsync_pulse_width(struct mxsfb_drm_private *mxsfb, u32 val)
> }
>
> /* Setup the MXSFB registers for decoding the pixels out of the framebuffer */
> -static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb)
> +static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb, bool update)
> {
> struct drm_crtc *crtc = &mxsfb->pipe.crtc;
> struct drm_device *drm = crtc->dev;
> const u32 format = crtc->primary->state->fb->format->format;
> - u32 ctrl, ctrl1;
> + u32 ctrl = 0, ctrl1 = 0;
> + bool bgr_format = true;
> + struct drm_format_name_buf format_name_buf;
>
> - ctrl = CTRL_BYPASS_COUNT | CTRL_MASTER;
> + if (!update)
> + ctrl = CTRL_BYPASS_COUNT | CTRL_MASTER;
>
> /*
> * WARNING: The bus width, CTRL_SET_BUS_WIDTH(), is configured to
> @@ -57,64 +60,158 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb)
> * to arbitrary value. This limitation should not pose an issue.
> */
>
> - /* CTRL1 contains IRQ config and status bits, preserve those. */
> - ctrl1 = readl(mxsfb->base + LCDC_CTRL1);
> - ctrl1 &= CTRL1_CUR_FRAME_DONE_IRQ_EN | CTRL1_CUR_FRAME_DONE_IRQ;
> + if (!update) {
> + /* CTRL1 contains IRQ config and status bits, preserve those. */
> + ctrl1 = readl(mxsfb->base + LCDC_CTRL1);
> + ctrl1 &= CTRL1_CUR_FRAME_DONE_IRQ_EN | CTRL1_CUR_FRAME_DONE_IRQ;
> + }
> +
> + DRM_DEV_DEBUG_DRIVER(drm->dev, "Setting up %s mode\n",
> + drm_get_format_name(format, &format_name_buf));
> +
> + /* Do some clean-up that we might have from a previous mode */
> + ctrl &= ~CTRL_SHIFT_DIR(1);
> + ctrl &= ~CTRL_SHIFT_NUM(0x3f);
> + if (mxsfb->devdata->ipversion >= 4)
> + writel(CTRL2_ODD_LINE_PATTERN(0x7) |
> + CTRL2_EVEN_LINE_PATTERN(0x7),
Would it make sense to not use magic constants here but rather '#define'
the different line pattern values as well?
> + mxsfb->base + LCDC_V4_CTRL2 + REG_CLR);
>
> switch (format) {
> - case DRM_FORMAT_RGB565:
> - dev_dbg(drm->dev, "Setting up RGB565 mode\n");
> + case DRM_FORMAT_BGR565: /* BG16 */
> + if (mxsfb->devdata->ipversion < 4)
> + goto err;
> + writel(CTRL2_ODD_LINE_PATTERN(0x5) |
> + CTRL2_EVEN_LINE_PATTERN(0x5),
> + mxsfb->base + LCDC_V4_CTRL2 + REG_SET);
> + /* Fall through */
> + case DRM_FORMAT_RGB565: /* RG16 */
> + ctrl |= CTRL_SET_WORD_LENGTH(0);
> + ctrl &= ~CTRL_DF16;
> + ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf);
> + break;
> + case DRM_FORMAT_XBGR1555: /* XB15 */
> + case DRM_FORMAT_ABGR1555: /* AB15 */
> + if (mxsfb->devdata->ipversion < 4)
> + goto err;
> + writel(CTRL2_ODD_LINE_PATTERN(0x5) |
> + CTRL2_EVEN_LINE_PATTERN(0x5),
> + mxsfb->base + LCDC_V4_CTRL2 + REG_SET);
> + /* Fall through */
> + case DRM_FORMAT_XRGB1555: /* XR15 */
> + case DRM_FORMAT_ARGB1555: /* AR15 */
> ctrl |= CTRL_SET_WORD_LENGTH(0);
> + ctrl |= CTRL_DF16;
> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf);
> break;
> - case DRM_FORMAT_XRGB8888:
> - dev_dbg(drm->dev, "Setting up XRGB8888 mode\n");
> + case DRM_FORMAT_RGBX8888: /* RX24 */
> + case DRM_FORMAT_RGBA8888: /* RA24 */
> + /* RGBX - > 0RGB */
> + ctrl |= CTRL_SHIFT_DIR(1);
> + ctrl |= CTRL_SHIFT_NUM(8);
> + bgr_format = false;
> + /* Fall through */
> + case DRM_FORMAT_XBGR8888: /* XB24 */
> + case DRM_FORMAT_ABGR8888: /* AB24 */
> + if (bgr_format) {
> + if (mxsfb->devdata->ipversion < 4)
> + goto err;
> + writel(CTRL2_ODD_LINE_PATTERN(0x5) |
> + CTRL2_EVEN_LINE_PATTERN(0x5),
> + mxsfb->base + LCDC_V4_CTRL2 + REG_SET);
> + }
> + /* Fall through */
> + case DRM_FORMAT_XRGB8888: /* XR24 */
> + case DRM_FORMAT_ARGB8888: /* AR24 */
> ctrl |= CTRL_SET_WORD_LENGTH(3);
> /* Do not use packed pixels = one pixel per word instead. */
> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7);
> break;
> default:
> - dev_err(drm->dev, "Unhandled pixel format %08x\n", format);
> - return -EINVAL;
> + goto err;
> }
>
> - writel(ctrl1, mxsfb->base + LCDC_CTRL1);
> - writel(ctrl, mxsfb->base + LCDC_CTRL);
> + if (update) {
> + writel(ctrl, mxsfb->base + LCDC_CTRL + REG_SET);
> + writel(ctrl1, mxsfb->base + LCDC_CTRL1 + REG_SET);
> + } else {
> + writel(ctrl, mxsfb->base + LCDC_CTRL);
> + writel(ctrl1, mxsfb->base + LCDC_CTRL1);
> + }
>
> return 0;
> +
> +err:
> + DRM_DEV_ERROR(drm->dev, "Unhandled pixel format: %s\n",
> + drm_get_format_name(format, &format_name_buf));
> +
> + return -EINVAL;
> +}
> +
> +static u32 get_bus_format_from_bpp(u32 bpp)
> +{
> + switch (bpp) {
> + case 16:
> + return MEDIA_BUS_FMT_RGB565_1X16;
> + case 18:
> + return MEDIA_BUS_FMT_RGB666_1X18;
> + case 24:
> + return MEDIA_BUS_FMT_RGB888_1X24;
> + default:
> + return MEDIA_BUS_FMT_RGB888_1X24;
> + }
> }
>
> static void mxsfb_set_bus_fmt(struct mxsfb_drm_private *mxsfb)
> {
> struct drm_crtc *crtc = &mxsfb->pipe.crtc;
> + unsigned int bits_per_pixel = crtc->primary->state->fb->format->depth;
> struct drm_device *drm = crtc->dev;
> u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> - u32 reg;
> -
> - reg = readl(mxsfb->base + LCDC_CTRL);
> + int num_bus_formats = mxsfb->connector->display_info.num_bus_formats;
> + const u32 *bus_formats = mxsfb->connector->display_info.bus_formats;
> + u32 reg = 0;
> + int i = 0;
> +
> + /* match the user requested bus_format to one supported by the panel */
> + if (num_bus_formats) {
> + u32 user_bus_format = get_bus_format_from_bpp(bits_per_pixel);
> +
> + bus_format = bus_formats[0];
> + for (i = 0; i < num_bus_formats; i++) {
> + if (user_bus_format == bus_formats[i]) {
> + bus_format = user_bus_format;
> + break;
> + }
> + }
> + }
>
> - if (mxsfb->connector->display_info.num_bus_formats)
> - bus_format = mxsfb->connector->display_info.bus_formats[0];
> + /*
> + * CRTC will dictate the bus format via private_flags[16:1]
> + * and private_flags[0] will signal a bus format change
> + */
> + crtc->mode.private_flags &= ~0x1FFFF; /* clear bus format */
> + crtc->mode.private_flags |= (bus_format << 1); /* set bus format */
> + crtc->mode.private_flags |= 0x1; /* bus format change indication*/
>
> DRM_DEV_DEBUG_DRIVER(drm->dev, "Using bus_format: 0x%08X\n",
> bus_format);
>
> - reg &= ~CTRL_BUS_WIDTH_MASK;
> switch (bus_format) {
> case MEDIA_BUS_FMT_RGB565_1X16:
> - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT);
> + reg = CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT);
> break;
> case MEDIA_BUS_FMT_RGB666_1X18:
> - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT);
> + reg = CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT);
> break;
> case MEDIA_BUS_FMT_RGB888_1X24:
> - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT);
> + reg = CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT);
> break;
> default:
> dev_err(drm->dev, "Unknown media bus format %d\n", bus_format);
> break;
> }
> - writel(reg, mxsfb->base + LCDC_CTRL);
> + writel(reg, mxsfb->base + LCDC_CTRL + REG_SET);
> }
>
> static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
> @@ -125,8 +222,9 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
> clk_prepare_enable(mxsfb->clk_disp_axi);
> clk_prepare_enable(mxsfb->clk);
>
> - writel(CTRL2_OUTSTANDING_REQS__REQ_16,
> - mxsfb->base + LCDC_V4_CTRL2 + REG_SET);
> + if (mxsfb->devdata->ipversion >= 4)
> + writel(CTRL2_OUTSTANDING_REQS__REQ_16,
> + mxsfb->base + LCDC_V4_CTRL2 + REG_SET);
>
> /* If it was disabled, re-enable the mode again */
> writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET);
> @@ -144,6 +242,10 @@ static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb)
> {
> u32 reg;
>
> + if (mxsfb->devdata->ipversion >= 4)
> + writel(CTRL2_OUTSTANDING_REQS(0x7),
> + mxsfb->base + LCDC_V4_CTRL2 + REG_CLR);
> +
> writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_CLR);
>
> /*
> @@ -231,7 +333,7 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
> /* Clear the FIFOs */
> writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);
>
> - err = mxsfb_set_pixel_fmt(mxsfb);
> + err = mxsfb_set_pixel_fmt(mxsfb, false);
> if (err)
> return;
>
> @@ -316,7 +418,7 @@ void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
> }
>
> void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
> - struct drm_plane_state *state)
> + struct drm_plane_state *old_state)
This hunk belongs to the previous commit.
> {
> struct drm_simple_display_pipe *pipe = &mxsfb->pipe;
> struct drm_crtc *crtc = &pipe->crtc;
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index 0d171e9..d9fb734 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -40,6 +40,27 @@ enum mxsfb_devtype {
> MXSFB_V4,
> };
>
> +/*
> + * When adding new formats, make sure to update the num_formats from
> + * mxsfb_devdata below.
> + */
> +static const u32 mxsfb_formats[] = {
> + /* MXSFB_V3 */
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_ARGB8888,
> + DRM_FORMAT_RGB565,
> + /* MXSFB_V4 */
> + DRM_FORMAT_XBGR8888,
> + DRM_FORMAT_ABGR8888,
> + DRM_FORMAT_RGBX8888,
> + DRM_FORMAT_RGBA8888,
> + DRM_FORMAT_ARGB1555,
> + DRM_FORMAT_XRGB1555,
> + DRM_FORMAT_ABGR1555,
> + DRM_FORMAT_XBGR1555,
> + DRM_FORMAT_BGR565
> +};
> +
> static const struct mxsfb_devdata mxsfb_devdata[] = {
> [MXSFB_V3] = {
> .transfer_count = LCDC_V3_TRANSFER_COUNT,
> @@ -49,6 +70,7 @@ static const struct mxsfb_devdata mxsfb_devdata[] = {
> .hs_wdth_mask = 0xff,
> .hs_wdth_shift = 24,
> .ipversion = 3,
> + .num_formats = 3,
> },
> [MXSFB_V4] = {
> .transfer_count = LCDC_V4_TRANSFER_COUNT,
> @@ -58,14 +80,10 @@ static const struct mxsfb_devdata mxsfb_devdata[] = {
> .hs_wdth_mask = 0x3fff,
> .hs_wdth_shift = 18,
> .ipversion = 4,
> + .num_formats = ARRAY_SIZE(mxsfb_formats),
> },
> };
>
> -static const uint32_t mxsfb_formats[] = {
> - DRM_FORMAT_XRGB8888,
> - DRM_FORMAT_RGB565
> -};
> -
> static struct mxsfb_drm_private *
> drm_pipe_to_mxsfb_drm_private(struct drm_simple_display_pipe *pipe)
> {
> @@ -241,7 +259,7 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags)
> }
>
> ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs,
> - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL,
> + mxsfb_formats, mxsfb->devdata->num_formats, NULL,
> mxsfb->connector);
> if (ret < 0) {
> dev_err(drm->dev, "Cannot setup simple display pipe\n");
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> index 0b65b51..8fb65d3 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> @@ -16,6 +16,7 @@ struct mxsfb_devdata {
> unsigned int hs_wdth_mask;
> unsigned int hs_wdth_shift;
> unsigned int ipversion;
> + unsigned int num_formats;
> };
>
> struct mxsfb_drm_private {
> @@ -42,6 +43,6 @@ void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb);
> void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb);
> void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb);
> void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
> - struct drm_plane_state *state);
> + struct drm_plane_state *old_state);
>
> #endif /* __MXSFB_DRV_H__ */
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> index 71426aa..9ee0d3c7 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> @@ -40,54 +40,76 @@
> #define LCDC_AS_BUF 0x220
> #define LCDC_AS_NEXT_BUF 0x230
>
> -#define CTRL_SFTRST (1 << 31)
> -#define CTRL_CLKGATE (1 << 30)
> -#define CTRL_BYPASS_COUNT (1 << 19)
> -#define CTRL_VSYNC_MODE (1 << 18)
> -#define CTRL_DOTCLK_MODE (1 << 17)
> -#define CTRL_DATA_SELECT (1 << 16)
> -#define CTRL_SET_BUS_WIDTH(x) (((x) & 0x3) << 10)
> -#define CTRL_GET_BUS_WIDTH(x) (((x) >> 10) & 0x3)
> -#define CTRL_BUS_WIDTH_MASK (0x3 << 10)
> -#define CTRL_SET_WORD_LENGTH(x) (((x) & 0x3) << 8)
> -#define CTRL_GET_WORD_LENGTH(x) (((x) >> 8) & 0x3)
> -#define CTRL_MASTER (1 << 5)
> -#define CTRL_DF16 (1 << 3)
> -#define CTRL_DF18 (1 << 2)
> -#define CTRL_DF24 (1 << 1)
> -#define CTRL_RUN (1 << 0)
> -
> -#define CTRL1_RECOVERY_ON_UNDERFLOW (1 << 24)
> -#define CTRL1_FIFO_CLEAR (1 << 21)
> -#define CTRL1_SET_BYTE_PACKAGING(x) (((x) & 0xf) << 16)
> -#define CTRL1_GET_BYTE_PACKAGING(x) (((x) >> 16) & 0xf)
> -#define CTRL1_CUR_FRAME_DONE_IRQ_EN (1 << 13)
> -#define CTRL1_CUR_FRAME_DONE_IRQ (1 << 9)
> -
> -#define CTRL2_OUTSTANDING_REQS__REQ_16 (4 << 21)
> +/* reg bit manipulation */
> +#define REG_MASK(e, s) (((1 << ((e) - (s) + 1)) - 1) << (s))
> +#define REG_PUT(x, e, s) (((x) << (s)) & REG_MASK(e, s))
> +#define REG_GET(x, e, s) (((x) & REG_MASK(e, s)) >> (s))
> +
> +#define SWIZZLE_LE 0 /* Little-Endian or No swap */
> +#define SWIZZLE_BE 1 /* Big-Endian or swap all */
> +#define SWIZZLE_HWD 2 /* Swap half-words */
> +#define SWIZZLE_HWD_BYTE 3 /* Swap bytes within each half-word */
> +
> +#define CTRL_SFTRST BIT(31)
> +#define CTRL_CLKGATE BIT(30)
> +#define CTRL_SHIFT_DIR(x) REG_PUT((x), 26, 26)
> +#define CTRL_SHIFT_NUM(x) REG_PUT((x), 25, 21)
> +#define CTRL_BYPASS_COUNT BIT(19)
> +#define CTRL_VSYNC_MODE BIT(18)
> +#define CTRL_DOTCLK_MODE BIT(17)
> +#define CTRL_DATA_SELECT BIT(16)
> +#define CTRL_INPUT_SWIZZLE(x) REG_PUT((x), 15, 14)
> +#define CTRL_CSC_SWIZZLE(x) REG_PUT((x), 13, 12)
> +#define CTRL_SET_BUS_WIDTH(x) REG_PUT((x), 11, 10)
> +#define CTRL_GET_BUS_WIDTH(x) REG_GET((x), 11, 10)
> +#define CTRL_BUS_WIDTH_MASK REG_PUT((0x3), 11, 10)
> +#define CTRL_SET_WORD_LENGTH(x) REG_PUT((x), 9, 8)
> +#define CTRL_GET_WORD_LENGTH(x) REG_GET((x), 9, 8)
> +#define CTRL_MASTER BIT(5)
> +#define CTRL_DF16 BIT(3)
> +#define CTRL_DF18 BIT(2)
> +#define CTRL_DF24 BIT(1)
> +#define CTRL_RUN BIT(0)
> +
> +#define CTRL1_RECOVERY_ON_UNDERFLOW BIT(24)
> +#define CTRL1_FIFO_CLEAR BIT(21)
> +#define CTRL1_SET_BYTE_PACKAGING(x) REG_PUT((x), 19, 16)
> +#define CTRL1_GET_BYTE_PACKAGING(x) REG_GET((x), 19, 16)
> +#define CTRL1_CUR_FRAME_DONE_IRQ_EN BIT(13)
> +#define CTRL1_CUR_FRAME_DONE_IRQ BIT(9)
Splitting the cleanups (introduction of BIT(x) usage) from new defines
would ease reviewing.
Cheers,
-- Guido
> +
> +#define REQ_1 0
> +#define REQ_2 1
> +#define REQ_4 2
> +#define REQ_8 3
> +#define REQ_16 4
> +
> +#define CTRL2_OUTSTANDING_REQS(x) REG_PUT((x), 23, 21)
> +#define CTRL2_ODD_LINE_PATTERN(x) REG_PUT((x), 18, 16)
> +#define CTRL2_EVEN_LINE_PATTERN(x) REG_PUT((x), 14, 12)
>
> #define TRANSFER_COUNT_SET_VCOUNT(x) (((x) & 0xffff) << 16)
> #define TRANSFER_COUNT_GET_VCOUNT(x) (((x) >> 16) & 0xffff)
> #define TRANSFER_COUNT_SET_HCOUNT(x) ((x) & 0xffff)
> #define TRANSFER_COUNT_GET_HCOUNT(x) ((x) & 0xffff)
>
> -#define VDCTRL0_ENABLE_PRESENT (1 << 28)
> -#define VDCTRL0_VSYNC_ACT_HIGH (1 << 27)
> -#define VDCTRL0_HSYNC_ACT_HIGH (1 << 26)
> -#define VDCTRL0_DOTCLK_ACT_FALLING (1 << 25)
> -#define VDCTRL0_ENABLE_ACT_HIGH (1 << 24)
> -#define VDCTRL0_VSYNC_PERIOD_UNIT (1 << 21)
> -#define VDCTRL0_VSYNC_PULSE_WIDTH_UNIT (1 << 20)
> -#define VDCTRL0_HALF_LINE (1 << 19)
> -#define VDCTRL0_HALF_LINE_MODE (1 << 18)
> +#define VDCTRL0_ENABLE_PRESENT BIT(28)
> +#define VDCTRL0_VSYNC_ACT_HIGH BIT(27)
> +#define VDCTRL0_HSYNC_ACT_HIGH BIT(26)
> +#define VDCTRL0_DOTCLK_ACT_FALLING BIT(25)
> +#define VDCTRL0_ENABLE_ACT_HIGH BIT(24)
> +#define VDCTRL0_VSYNC_PERIOD_UNIT BIT(21)
> +#define VDCTRL0_VSYNC_PULSE_WIDTH_UNIT BIT(20)
> +#define VDCTRL0_HALF_LINE BIT(19)
> +#define VDCTRL0_HALF_LINE_MODE BIT(18)
> #define VDCTRL0_SET_VSYNC_PULSE_WIDTH(x) ((x) & 0x3ffff)
> #define VDCTRL0_GET_VSYNC_PULSE_WIDTH(x) ((x) & 0x3ffff)
>
> #define VDCTRL2_SET_HSYNC_PERIOD(x) ((x) & 0x3ffff)
> #define VDCTRL2_GET_HSYNC_PERIOD(x) ((x) & 0x3ffff)
>
> -#define VDCTRL3_MUX_SYNC_SIGNALS (1 << 29)
> -#define VDCTRL3_VSYNC_ONLY (1 << 28)
> +#define VDCTRL3_MUX_SYNC_SIGNALS BIT(29)
> +#define VDCTRL3_VSYNC_ONLY BIT(28)
> #define SET_HOR_WAIT_CNT(x) (((x) & 0xfff) << 16)
> #define GET_HOR_WAIT_CNT(x) (((x) >> 16) & 0xfff)
> #define SET_VERT_WAIT_CNT(x) ((x) & 0xffff)
> @@ -95,7 +117,7 @@
>
> #define VDCTRL4_SET_DOTCLK_DLY(x) (((x) & 0x7) << 29) /* v4 only */
> #define VDCTRL4_GET_DOTCLK_DLY(x) (((x) >> 29) & 0x7) /* v4 only */
> -#define VDCTRL4_SYNC_SIGNALS_ON (1 << 18)
> +#define VDCTRL4_SYNC_SIGNALS_ON BIT(18)
> #define SET_DOTCLK_H_VALID_DATA_CNT(x) ((x) & 0x3ffff)
>
> #define DEBUG0_HSYNC (1 < 26)
> @@ -116,7 +138,7 @@
> #define STMLCDIF_18BIT 2 /* pixel data bus to the display is of 18 bit width */
> #define STMLCDIF_24BIT 3 /* pixel data bus to the display is of 24 bit width */
>
> -#define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT (1 << 6)
> -#define MXSFB_SYNC_DOTCLK_FALLING_ACT (1 << 7) /* negative edge sampling */
> +#define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT BIT(6)
> +#define MXSFB_SYNC_DOTCLK_FALLING_ACT BIT(7) /* negative edge sampling */
>
> #endif /* __MXSFB_REGS_H__ */
> --
> 2.7.4
>
>
> _______________________________________________
> 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 v2 1/2] arm64: dts: imx8mq: Add MIPI D-PHY
From: Guido Günther @ 2019-07-21 11:47 UTC (permalink / raw)
To: Abel Vesa
Cc: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, dl-linux-imx,
Pavel Machek, Angus Ainslie (Purism), Lucas Stach, Anson Huang,
Carlo Caione, Andrey Smirnov, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190715111027.a4wlpzex3taxymyr@fsr-ub1664-175>
Hi Abel,
On Mon, Jul 15, 2019 at 11:10:27AM +0000, Abel Vesa wrote:
> On 19-07-15 12:43:05, Guido Günther wrote:
> > Add a node for the Mixel MIPI D-PHY, "disabled" by default.
> >
> > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > Acked-by: Angus Ainslie (Purism) <angus@akkea.ca>
> > ---
> > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > index d09b808eff87..891ee7578c2d 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > @@ -728,6 +728,19 @@
> > status = "disabled";
> > };
> >
> > + dphy: dphy@30a00300 {
> > + compatible = "fsl,imx8mq-mipi-dphy";
> > + reg = <0x30a00300 0x100>;
> > + clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
> > + clock-names = "phy_ref";
> > + assigned-clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
> > + assigned-clock-parents = <&clk IMX8MQ_VIDEO_PLL1_OUT>;
> > + assigned-clock-rates = <24000000>;
>
> We have the following in the clk-imx8mq in the vendor tree:
>
> clk_set_parent(clks[IMX8MQ_VIDEO_PLL1_BYPASS], clks[IMX8MQ_VIDEO_PLL1]);
>
> This unbypasses the video pll 1. And then we also have this:
>
> /* config video_pll1 clock */
> clk_set_parent(clks[IMX8MQ_VIDEO_PLL1_REF_SEL], clks[IMX8MQ_CLK_27M]);
> clk_set_rate(clks[IMX8MQ_VIDEO_PLL1], 593999999);
We don't have anything like this in our tree. This is our current clock
tree in that area which is setup by either the lcdif or dcss DT:
osc_25m 10 12 0 25000000 0 0 50000
video_pll1_ref_sel 1 1 0 25000000 0 0 50000
video_pll1_ref_div 1 1 0 5000000 0 0 50000
video_pll1 1 1 0 593999998 0 0 50000
video_pll1_bypass 1 1 0 593999998 0 0 50000
video_pll1_out 2 2 0 593999998 0 0 50000
dsi_phy_ref 1 1 0 23760000 0 0 50000
e.g. for lcdif we have:
lcdif: lcdif@30320000 {
...
clock-names = "pix";
assigned-clocks = <&clk IMX8MQ_VIDEO_PLL1_REF_SEL>,
<&clk IMX8MQ_VIDEO_PLL1_BYPASS>,
<&clk IMX8MQ_CLK_LCDIF_PIXEL>,
<&clk IMX8MQ_VIDEO_PLL1>;
assigned-clock-parents = <&clk IMX8MQ_CLK_25M>,
<&clk IMX8MQ_VIDEO_PLL1>,
<&clk IMX8MQ_VIDEO_PLL1_OUT>;
assigned-clock-rates = <0>, <0>, <0>, <594000000>;
...
};
Do we want to add this add for dphy and lcdif?
Cheers,
-- Guido
> But none of that is acceptable upstream since the clock provider should not
> use clock consumer API.
>
> So please update the assigned-clock* properties to something like this:
> assigned-clocks = <&clk IMX8MQ_VIDEO_PLL1_REF_SEL>,
> <&clk IMX8MQ_VIDEO_PLL1_BYPASS>,
> <&clk IMX8MQ_CLK_DSI_PHY_REF>,
> <&clk IMX8MQ_VIDEO_PLL1>;
> assigned-clock-parents = <&clk IMX8MQ_CLK_27M>,
> <&clk IMX8MQ_VIDEO_PLL1>,
> <&clk IMX8MQ_VIDEO_PLL1_OUT>
> <0>;
> assigned-clock-rates = <0>,
> <0>,
> <24000000>,
> <593999999>;
>
> I've written this without testing, so please do test it on your setup.
>
> > + #phy-cells = <0>;
> > + power-domains = <&pgc_mipi>;
> > + status = "disabled";
> > + };
> > +
> > i2c1: i2c@30a20000 {
> > compatible = "fsl,imx8mq-i2c", "fsl,imx21-i2c";
> > reg = <0x30a20000 0x10000>;
> > --
> > 2.20.1
> >
^ permalink raw reply
* Re: [Question] orphan platform data header
From: Masahiro Yamada @ 2019-07-21 12:12 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ben Dooks, Linux Kernel Mailing List, Linus Torvalds,
Greg Kroah-Hartman, DTML, linux-arm-kernel
In-Reply-To: <CAK8P3a3cURmbGZc-6ESLjrF465VLnBroD4QENyfsSsCrNenRrA@mail.gmail.com>
On Sun, Jul 21, 2019 at 6:10 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sun, Jul 21, 2019 at 5:45 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> > On Sat, Jul 20, 2019 at 10:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > > > So, what shall we do?
> > > >
> > > > Drop the board-file support? Or, keep it
> > > > in case somebody is still using their board-files
> > > > in downstream?
> >>
> > > For this file, all boards got converted to DT, and the old setup
> > > code removed in commit ebc278f15759 ("ARM: mvebu: remove static
> > > LED setup for netxbig boards"), four years ago, so it's a fairly
> > > easy decision to make it DT only.
> >
> > I see another case, which is difficult
> > to make a decision.
> >
> > For example, drivers/spi/spi-tle62x0.c
> >
> > This driver supports only board-file, but the board-file
> > is not found in upstream.
> >
> > Unless I am terribly missing something,
> > there is no one who passes tle62x0_pdata
> > to this driver.
> >
> > $ git grep tle62x0_pdata
> > drivers/spi/spi-tle62x0.c: struct tle62x0_pdata *pdata;
> > include/linux/spi/tle62x0.h:struct tle62x0_pdata {
> >
> > But, removing board-file support
> > makes this driver completely useless...
>
> Adding Ben Dooks to Cc.
>
> I suspect this driver is completely obsolete and should be removed.
>
> For some reason, it's not an SPI controller driver like all the other
> files in that directory, but implements low-level access to the state
> of a particular SPI device.
>
> However, there should not really be a low-level driver for it that
> just exports the pins to user space. It should either be a gpiolib
> driver to let other drivers talk to the pins, or a high-level driver that
> exposes the intended functionality (watchdog, regulator, ...)
> to those respective subsystems.
>
> Arnd
Another example that I have no idea
how it works:
drivers/net/hamradio/yam.c
yam_ioctl() reads data from user-space,
but the data structures for ioctl are
defined in include/linux/yam.h
This header is not exported to user-space
since it is outside of the uapi directory.
I dug the git history, but it has never
exported to user-space in the past.
I do not know how user-space programs can
pass-in data to the kernel.
If we want to fix this, we could move it
to include/uapi/linux/yam.h
But, if nobody has reported any problem about this,
it might be a good proof that nobody is using this driver.
Maybe, can we simply drop odd drivers??
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [Question] orphan platform data header
From: Arnd Bergmann @ 2019-07-21 14:15 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Ben Dooks, Linux Kernel Mailing List, Linus Torvalds,
Greg Kroah-Hartman, DTML, linux-arm-kernel
In-Reply-To: <CAK7LNARN=iNmresDJ2=J1wOb2QYoZ7yw4O0Q9sYVPo0jRKDf=w@mail.gmail.com>
On Sun, Jul 21, 2019 at 2:13 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> On Sun, Jul 21, 2019 at 6:10 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sun, Jul 21, 2019 at 5:45 AM Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> > > On Sat, Jul 20, 2019 at 10:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>
> Another example that I have no idea
> how it works:
>
> drivers/net/hamradio/yam.c
>
> yam_ioctl() reads data from user-space,
> but the data structures for ioctl are
> defined in include/linux/yam.h
That is different: the hardware attaches to a serial port and may well
be usable, and the user space side just contains a copy of the header,
see https://github.com/nwdigitalradio/ax25-tools/tree/master/yamdrv
> If we want to fix this, we could move it
> to include/uapi/linux/yam.h
We could do that, or just leave it alone, as nobody would
tell the difference.
> But, if nobody has reported any problem about this,
> it might be a good proof that nobody is using this driver.
>
> Maybe, can we simply drop odd drivers??
Both the kernel driver and the user space side have a maintainer, and
I see no indication that it is actually broken. The driver is clearly
a relic from old times (earlier than 2.4) and we would not merge
this driver today.
It seems more useful to keep looking for drivers with a platform_data
header file that is no longer included by any platform for candidates
that may be obsolete.
Arnd
^ permalink raw reply
* Re: [PATCHv8 5/5] coresight: cpu-debug: Add support for Qualcomm Kryo
From: Leo Yan @ 2019-07-21 14:35 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Sai Prakash Ranjan, Greg Kroah-Hartman, Suzuki K Poulose,
Alexander Shishkin, Mike Leach, Rob Herring, Bjorn Andersson,
devicetree, David Brown, Mark Rutland, Rajendra Nayak,
Vivek Gautam, Sibi Sankar, linux-arm-kernel, linux-kernel,
linux-arm-msm, Marc Gonzalez
In-Reply-To: <20190717165602.GA4271@xps15>
On Wed, Jul 17, 2019 at 10:56:02AM -0600, Mathieu Poirier wrote:
> On Fri, Jul 12, 2019 at 07:46:27PM +0530, Sai Prakash Ranjan wrote:
> > Add support for coresight CPU debug module on Qualcomm
> > Kryo CPUs. This patch adds the UCI entries for Kryo CPUs
> > found on MSM8996 which shares the same PIDs as ETMs.
> >
> > Without this, below error is observed on MSM8996:
> >
> > [ 5.429867] OF: graph: no port node found in /soc/debug@3810000
> > [ 5.429938] coresight-etm4x: probe of 3810000.debug failed with error -22
> > [ 5.435415] coresight-cpu-debug 3810000.debug: Coresight debug-CPU0 initialized
> > [ 5.446474] OF: graph: no port node found in /soc/debug@3910000
> > [ 5.448927] coresight-etm4x: probe of 3910000.debug failed with error -22
> > [ 5.454681] coresight-cpu-debug 3910000.debug: Coresight debug-CPU1 initialized
> > [ 5.487765] OF: graph: no port node found in /soc/debug@3a10000
> > [ 5.488007] coresight-etm4x: probe of 3a10000.debug failed with error -22
> > [ 5.493024] coresight-cpu-debug 3a10000.debug: Coresight debug-CPU2 initialized
> > [ 5.501802] OF: graph: no port node found in /soc/debug@3b10000
> > [ 5.512901] coresight-etm4x: probe of 3b10000.debug failed with error -22
> > [ 5.513192] coresight-cpu-debug 3b10000.debug: Coresight debug-CPU3 initialized
> >
> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> > ---
> > .../hwtracing/coresight/coresight-cpu-debug.c | 33 +++++++++----------
> > drivers/hwtracing/coresight/coresight-priv.h | 10 +++---
> > 2 files changed, 21 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> > index 2463aa7ab4f6..96544b348c27 100644
> > --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
> > +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> > @@ -646,24 +646,23 @@ static int debug_remove(struct amba_device *adev)
> > return 0;
> > }
> >
> > +static const struct amba_cs_uci_id uci_id_debug[] = {
> > + {
> > + /* CPU Debug UCI data */
> > + .devarch = 0x47706a15,
> > + .devarch_mask = 0xfff0ffff,
> > + .devtype = 0x00000015,
> > + }
> > +};
> > +
> > static const struct amba_id debug_ids[] = {
> > - { /* Debug for Cortex-A53 */
> > - .id = 0x000bbd03,
> > - .mask = 0x000fffff,
> > - },
> > - { /* Debug for Cortex-A57 */
> > - .id = 0x000bbd07,
> > - .mask = 0x000fffff,
> > - },
> > - { /* Debug for Cortex-A72 */
> > - .id = 0x000bbd08,
> > - .mask = 0x000fffff,
> > - },
> > - { /* Debug for Cortex-A73 */
> > - .id = 0x000bbd09,
> > - .mask = 0x000fffff,
> > - },
> > - { 0, 0 },
> > + CS_AMBA_ID(0x000bbd03), /* Cortex-A53 */
> > + CS_AMBA_ID(0x000bbd07), /* Cortex-A57 */
> > + CS_AMBA_ID(0x000bbd08), /* Cortex-A72 */
> > + CS_AMBA_ID(0x000bbd09), /* Cortex-A73 */
> > + CS_AMBA_UCI_ID(0x000f0205, uci_id_debug), /* Qualcomm Kryo */
> > + CS_AMBA_UCI_ID(0x000f0211, uci_id_debug), /* Qualcomm Kryo */
> > + {},
> > };
> >
> > static struct amba_driver debug_driver = {
> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> > index 7d401790dd7e..41ae5863104d 100644
> > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> > @@ -185,11 +185,11 @@ static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
> > }
> >
> > /* coresight AMBA ID, full UCI structure: id table entry. */
> > -#define CS_AMBA_UCI_ID(pid, uci_ptr) \
> > - { \
> > - .id = pid, \
> > - .mask = 0x000fffff, \
> > - .data = uci_ptr \
> > +#define CS_AMBA_UCI_ID(pid, uci_ptr) \
> > + { \
> > + .id = pid, \
> > + .mask = 0x000fffff, \
> > + .data = (void *)uci_ptr \
> > }
>
> I will pickup this patch - it will show up in my next tree when rc1 comes out.
I tested this patch on the mainline kernel with latest commit
f1a3b43cc1f5 ("Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input"). FWIW:
Tested-by: Leo Yan <leo.yan@linaro.org>
P.s. Acutally I tested this patch for 5.2-rcx a few days ago and found
a regression for CPU debug module: I observed the CPU debug module
panic dump will stuck. After I pulled to latest kernel code base the
CPU debug module can work well; also works well with this patch. F.Y.I.
Thanks,
Leo Yan
^ permalink raw reply
* Re: [Question] orphan platform data header
From: Masahiro Yamada @ 2019-07-21 14:58 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ben Dooks, Linux Kernel Mailing List, Linus Torvalds,
Greg Kroah-Hartman, DTML, linux-arm-kernel
In-Reply-To: <CAK8P3a133ekPqkLWfC2ee0mT3iLbFzSjJ9FDokSyaX+hMVigKA@mail.gmail.com>
On Sun, Jul 21, 2019 at 11:15 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sun, Jul 21, 2019 at 2:13 PM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> > On Sun, Jul 21, 2019 at 6:10 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Sun, Jul 21, 2019 at 5:45 AM Masahiro Yamada
> > > <yamada.masahiro@socionext.com> wrote:
> > > > On Sat, Jul 20, 2019 at 10:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >
> > Another example that I have no idea
> > how it works:
> >
> > drivers/net/hamradio/yam.c
> >
> > yam_ioctl() reads data from user-space,
> > but the data structures for ioctl are
> > defined in include/linux/yam.h
>
> That is different: the hardware attaches to a serial port and may well
> be usable, and the user space side just contains a copy of the header,
> see https://github.com/nwdigitalradio/ax25-tools/tree/master/yamdrv
Oh, I did not know that
user-space had a copy of that.
> > If we want to fix this, we could move it
> > to include/uapi/linux/yam.h
>
> We could do that, or just leave it alone, as nobody would
> tell the difference.
When we are changing structures in uapi,
it is very likely a red alert.
On the other hand, we can change code outside of uapi
more safely.
One benefit of uapi is we can catch the compatibility level
from the directory path.
>
> > But, if nobody has reported any problem about this,
> > it might be a good proof that nobody is using this driver.
> >
> > Maybe, can we simply drop odd drivers??
>
> Both the kernel driver and the user space side have a maintainer, and
> I see no indication that it is actually broken. The driver is clearly
> a relic from old times (earlier than 2.4) and we would not merge
> this driver today.
>
> It seems more useful to keep looking for drivers with a platform_data
> header file that is no longer included by any platform for candidates
> that may be obsolete.
OK.
Thanks.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v2 6/8] PCI: al: Add support for DW based driver type
From: Chocron, Jonathan @ 2019-07-21 15:08 UTC (permalink / raw)
To: jingoohan1@gmail.com, mark.rutland@arm.com,
lorenzo.pieralisi@arm.com, Gustavo.Pimentel@synopsys.com,
bhelgaas@google.com, robh+dt@kernel.org
Cc: linux-kernel@vger.kernel.org, Woodhouse, David, Hanoch, Uri,
devicetree@vger.kernel.org, Wasserstrom, Barak, Saidi, Ali,
Hawa, Hanna, Shenhar, Talel, Krupnik, Ronen,
linux-pci@vger.kernel.org, benh@kernel.crashing.org
In-Reply-To: <DM6PR12MB4010913E5408349A600762CADACB0@DM6PR12MB4010.namprd12.prod.outlook.com>
On Fri, 2019-07-19 at 08:55 +0000, Gustavo Pimentel wrote:
> On Thu, Jul 18, 2019 at 10:47:16, Jonathan Chocron <jonnyc@amazon.com
> >
> wrote:
>
> > This driver is DT based and utilizes the DesignWare APIs.
> > It allows using a smaller ECAM range for a larger bus range -
> > usually an entire bus uses 1MB of address space, but the driver
> > can use it for a larger number of buses.
> >
> > All link initializations are handled by the boot FW.
> >
> > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > ---
> > drivers/pci/controller/dwc/Kconfig | 12 +
> > drivers/pci/controller/dwc/pcie-al.c | 373
> > +++++++++++++++++++++++++++
> > 2 files changed, 385 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig
> > b/drivers/pci/controller/dwc/Kconfig
> > index 6ea778ae4877..3c6094cbcc3b 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -230,4 +230,16 @@ config PCIE_UNIPHIER
> > Say Y here if you want PCIe controller support on UniPhier
> > SoCs.
> > This driver supports LD20 and PXs3 SoCs.
> >
> > +config PCIE_AL
> > + bool "Amazon Annapurna Labs PCIe controller"
> > + depends on OF && (ARM64 || COMPILE_TEST)
> > + depends on PCI_MSI_IRQ_DOMAIN
> > + select PCIE_DW_HOST
> > + help
> > + Say Y here to enable support of the Amazon's Annapurna Labs
> > PCIe
> > + controller IP on Amazon SoCs. The PCIe controller uses the
> > DesignWare
> > + core plus Annapurna Labs proprietary hardware wrappers. This
> > is
> > + required only for DT-based platforms. ACPI platforms with the
> > + Annapurna Labs PCIe controller don't need to enable this.
> > +
> > endmenu
> > diff --git a/drivers/pci/controller/dwc/pcie-al.c
> > b/drivers/pci/controller/dwc/pcie-al.c
> > index 3ab58f0584a8..40555532fb9a 100644
> > --- a/drivers/pci/controller/dwc/pcie-al.c
> > +++ b/drivers/pci/controller/dwc/pcie-al.c
> > @@ -91,3 +91,376 @@ struct pci_ecam_ops al_pcie_ops = {
> > };
> >
> > #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> > +
> > +#ifdef CONFIG_PCIE_AL
> > +
> > +#include <linux/of_pci.h>
> > +#include "pcie-designware.h"
> > +
> > +#define AL_PCIE_REV_ID_2 2
> > +#define AL_PCIE_REV_ID_3 3
> > +#define AL_PCIE_REV_ID_4 4
> > +
> > +#define AXI_BASE_OFFSET 0x0
> > +
> > +#define DEVICE_ID_OFFSET 0x16c
> > +
> > +#define DEVICE_REV_ID 0x0
> > +#define DEVICE_REV_ID_DEV_ID_MASK GENMASK(31, 16)
> > +
> > +#define DEVICE_REV_ID_DEV_ID_X4 0
> > +#define DEVICE_REV_ID_DEV_ID_X8 2
> > +#define DEVICE_REV_ID_DEV_ID_X16 4
> > +
> > +#define OB_CTRL_REV1_2_OFFSET 0x0040
> > +#define OB_CTRL_REV3_5_OFFSET 0x0030
> > +
> > +#define CFG_TARGET_BUS 0x0
> > +#define CFG_TARGET_BUS_MASK_MASK GENMASK(7, 0)
> > +#define CFG_TARGET_BUS_BUSNUM_MASK GENMASK(15, 8)
> > +
> > +#define CFG_CONTROL 0x4
> > +#define CFG_CONTROL_SUBBUS_MASK GENMASK(15, 8)
> > +#define CFG_CONTROL_SEC_BUS_MASK GENMASK(23, 16)
> > +
> > +struct al_pcie_reg_offsets {
> > + unsigned int ob_ctrl;
> > +};
> > +
> > +struct al_pcie_target_bus_cfg {
> > + u8 reg_val;
> > + u8 reg_mask;
> > + u8 ecam_mask;
> > +};
> > +
> > +struct al_pcie {
> > + struct dw_pcie *pci;
> > + void __iomem *controller_base; /* base of PCIe unit (not DW
> > core) */
> > + struct device *dev;
> > + resource_size_t ecam_size;
> > + unsigned int controller_rev_id;
> > + struct al_pcie_reg_offsets reg_offsets;
> > + struct al_pcie_target_bus_cfg target_bus_cfg;
> > +};
> > +
> > +#define PCIE_ECAM_DEVFN(x) (((x) & 0xff) << 12)
> > +
> > +#define to_al_pcie(x) dev_get_drvdata((x)->dev)
> > +
> > +static int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int
> > *rev_id)
> > +{
> > + void __iomem *dev_rev_id_addr;
> > + u32 dev_rev_id;
> > +
> > + dev_rev_id_addr = (void __iomem *)((uintptr_t)pcie-
> > >controller_base +
> > + AXI_BASE_OFFSET + DEVICE_ID_OFFSET +
> > DEVICE_REV_ID);
> > +
> > + dev_rev_id = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK,
> > + readl(dev_rev_id_addr));
> > + switch (dev_rev_id) {
> > + case DEVICE_REV_ID_DEV_ID_X4:
> > + *rev_id = AL_PCIE_REV_ID_2;
> > + break;
> > + case DEVICE_REV_ID_DEV_ID_X8:
> > + *rev_id = AL_PCIE_REV_ID_3;
> > + break;
> > + case DEVICE_REV_ID_DEV_ID_X16:
> > + *rev_id = AL_PCIE_REV_ID_4;
> > + break;
> > + default:
> > + dev_err(pcie->dev, "Unsupported dev_rev_id (0x%x)\n",
> > + dev_rev_id);
> > + return -EINVAL;
> > + }
> > +
> > + dev_dbg(pcie->dev, "dev_rev_id: 0x%x\n", dev_rev_id);
>
> Consider s/dev_dbg/pci_dbg/g
>
There is no struct pci_dev context (the dev belongs to the
platform_device).
> > +
> > + return 0;
> > +}
> > +
> > +static int al_pcie_reg_offsets_set(struct al_pcie *pcie)
> > +{
> > + switch (pcie->controller_rev_id) {
> > + case AL_PCIE_REV_ID_2:
> > + pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
> > + break;
> > + case AL_PCIE_REV_ID_3:
> > + case AL_PCIE_REV_ID_4:
> > + pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
> > + break;
> > + default:
> > + dev_err(pcie->dev, "Unsupported controller rev_id:
> > 0x%x\n",
> > + pcie->controller_rev_id);
>
> Consider s/dev_err/pci_err/g
>
Same as above.
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
> > + u8 target_bus,
> > + u8 mask_target_bus)
> > +{
> > + void __iomem *cfg_control_addr;
> > + u32 reg;
> > +
> > + reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK, mask_target_bus) |
> > + FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK, target_bus);
> > +
> > + cfg_control_addr = (void __iomem *)((uintptr_t)pcie-
> > >controller_base +
> > + AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl
> > +
> > + CFG_TARGET_BUS);
> > +
> > + writel(reg, cfg_control_addr);
>
> From what I'm seeing you commonly use writel() and readl() with a
> common
> base address, such as pcie->controller_base + AXI_BASE_OFFSET.
> I'd suggest to creating a writel and readl with that offset built-in.
>
I prefer to keep it generic, since in future revisions we might want to
access regs which are not in the AXI region. You think I should add
wrappers which simply hide the pcie->controller_base part?
> > +}
> > +
> > +static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
> > + unsigned int busnr,
> > + unsigned int devfn)
> > +{
> > + void __iomem *pci_base_addr;
>
> Consider passing this variable declaration to the bottom, following
> the
> reverse tree order.
>
Done. Moved 'struct pcie_port *pp' as well.
> > + struct pcie_port *pp = &pcie->pci->pp;
> > + struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie-
> > >target_bus_cfg;
> > + unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
> > + unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
> > +
> > + pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
> > + (busnr_ecam << 20) +
> > + PCIE_ECAM_DEVFN(devfn));
> > +
> > + if (busnr_reg != target_bus_cfg->reg_val) {
> > + dev_dbg(pcie->pci->dev, "Changing target bus busnum val
> > from 0x%x to 0x%x\n",
> > + target_bus_cfg->reg_val, busnr_reg);
> > + target_bus_cfg->reg_val = busnr_reg;
> > + al_pcie_target_bus_set(pcie,
> > + target_bus_cfg->reg_val,
> > + target_bus_cfg->reg_mask);
> > + }
> > +
> > + return pci_base_addr;
> > +}
> > +
> > +static int al_pcie_rd_other_conf(struct pcie_port *pp, struct
> > pci_bus *bus,
> > + unsigned int devfn, int where, int
> > size,
> > + u32 *val)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct al_pcie *pcie = to_al_pcie(pci);
> > + unsigned int busnr = bus->number;
> > + void __iomem *pci_addr;
> > + int rc;
> > +
> > + pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
> > +
> > + rc = dw_pcie_read(pci_addr + where, size, val);
> > +
> > + dev_dbg(pci->dev, "%d-byte config read from %04x:%02x:%02x.%d
> > offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
> > + size, pci_domain_nr(bus), bus->number,
> > + PCI_SLOT(devfn), PCI_FUNC(devfn), where,
> > + (pci_addr + where), *val);
> > +
> > + return rc;
> > +}
> > +
> > +static int al_pcie_wr_other_conf(struct pcie_port *pp, struct
> > pci_bus *bus,
> > + unsigned int devfn, int where, int
> > size,
> > + u32 val)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct al_pcie *pcie = to_al_pcie(pci);
> > + unsigned int busnr = bus->number;
> > + void __iomem *pci_addr;
> > + int rc;
> > +
> > + pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
> > +
> > + rc = dw_pcie_write(pci_addr + where, size, val);
> > +
> > + dev_err(pci->dev, "%d-byte config write to %04x:%02x:%02x.%d
> > offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
> > + size, pci_domain_nr(bus), bus->number,
> > + PCI_SLOT(devfn), PCI_FUNC(devfn), where,
> > + (pci_addr + where), val);
> > +
> > + return rc;
> > +}
> > +
> > +static int al_pcie_config_prepare(struct al_pcie *pcie)
> > +{
> > + struct al_pcie_target_bus_cfg *target_bus_cfg;
> > + struct pcie_port *pp = &pcie->pci->pp;
> > + unsigned int ecam_bus_mask;
> > + u8 secondary_bus;
> > + u8 subordinate_bus;
> > + void __iomem *cfg_control_addr;
> > + u32 cfg_control;
> > + u32 reg;
> > +
> > + target_bus_cfg = &pcie->target_bus_cfg;
> > +
> > + ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
> > + if (ecam_bus_mask > 255) {
> > + dev_warn(pcie->dev, "ECAM window size is larger than
> > 256MB. Cutting off at 256\n");
> > + ecam_bus_mask = 255;
> > + }
> > +
> > + /* This portion is taken from the transaction address */
> > + target_bus_cfg->ecam_mask = ecam_bus_mask;
> > + /* This portion is taken from the cfg_target_bus reg */
> > + target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
> > + target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg-
> > >reg_mask;
> > +
> > + al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
> > + target_bus_cfg->reg_mask);
> > +
> > + secondary_bus = pp->busn->start + 1;
> > + subordinate_bus = pp->busn->end;
> > +
> > + /* Set the valid values of secondary and subordinate buses */
> > + cfg_control_addr = pcie->controller_base + AXI_BASE_OFFSET +
> > + pcie->reg_offsets.ob_ctrl + CFG_CONTROL;
> > +
> > + cfg_control = readl(cfg_control_addr);
> > +
> > + reg = cfg_control &
> > + ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
> > +
> > + reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
> > + FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
> > +
> > + writel(reg, cfg_control_addr);
> > +
> > + return 0;
> > +}
> > +
> > +static int al_pcie_host_init(struct pcie_port *pp)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct al_pcie *pcie = to_al_pcie(pci);
> > + int link_up;
> > + int rc;
> > +
> > + link_up = dw_pcie_link_up(pci);
> > + if (!link_up) {
> > + dev_err(pci->dev, "link is not up!\n");
> > + return -ENOLINK;
> > + }
> > +
> > + dev_info(pci->dev, "link is up\n");
>
> Consider s/dev_info/pci_info/g
>
Same as the response above.
> > +
> > + rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
> > + if (rc)
> > + return rc;
> > +
> > + rc = al_pcie_reg_offsets_set(pcie);
> > + if (rc)
> > + return rc;
> > +
> > + rc = al_pcie_config_prepare(pcie);
> > + if (rc)
> > + return rc;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dw_pcie_host_ops al_pcie_host_ops = {
> > + .rd_other_conf = al_pcie_rd_other_conf,
> > + .wr_other_conf = al_pcie_wr_other_conf,
> > + .host_init = al_pcie_host_init,
> > +};
> > +
> > +static int al_add_pcie_port(struct pcie_port *pp,
> > + struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + pp->ops = &al_pcie_host_ops;
> > +
> > + ret = dw_pcie_host_init(pp);
> > + if (ret) {
> > + dev_err(dev, "failed to initialize host\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dw_pcie_ops dw_pcie_ops = {
> > +};
> > +
> > +static int al_pcie_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct al_pcie *al_pcie;
> > + struct dw_pcie *pci;
> > + struct resource *dbi_res;
> > + struct resource *controller_res;
> > + struct resource *ecam_res;
> > + int ret;
>
> Please sort the variables following the reverse tree order.
>
Done.
I'd think that it would make sense to group variables which have a
common characteristic (e.g. resources read from the DT), even if it
mildly breaks the convention (as long as the general frame is longest
to shortest). Does this sound ok?
BTW, I couldn't find any documentation regarding the reverse-tree
convention, do you have a pointer to some?
> > +
> > + al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
> > + if (!al_pcie)
> > + return -ENOMEM;
> > +
> > + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > + if (!pci)
> > + return -ENOMEM;
> > +
> > + pci->dev = dev;
> > + pci->ops = &dw_pcie_ops;
> > +
> > + al_pcie->pci = pci;
> > + al_pcie->dev = dev;
> > +
> > + dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "dbi");
> > + pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_res);
> > + if (IS_ERR(pci->dbi_base)) {
> > + dev_err(dev, "couldn't remap dbi base %pR\n", dbi_res);
> > + return PTR_ERR(pci->dbi_base);
> > + }
> > +
> > + ecam_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "config");
> > + if (!ecam_res) {
> > + dev_err(dev, "couldn't find 'config' reg in DT\n");
> > + return -ENOENT;
> > + }
> > + al_pcie->ecam_size = resource_size(ecam_res);
> > +
> > + controller_res = platform_get_resource_byname(pdev,
> > IORESOURCE_MEM,
> > + "controller");
> > + al_pcie->controller_base = devm_ioremap_resource(dev,
> > controller_res);
> > + if (IS_ERR(al_pcie->controller_base)) {
> > + dev_err(dev, "couldn't remap controller base %pR\n",
> > + controller_res);
> > + return PTR_ERR(al_pcie->controller_base);
> > + }
> > +
> > + dev_dbg(dev, "From DT: dbi_base: %pR, controller_base: %pR\n",
> > + dbi_res, controller_res);
> > +
> > + platform_set_drvdata(pdev, al_pcie);
> > +
> > + ret = al_add_pcie_port(&pci->pp, pdev);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> Those operations are redundant, aren't they? They can be replaced
> just
> by:
>
> return ret;
>
Ack.
> > +}
> > +
> > +static const struct of_device_id al_pcie_of_match[] = {
> > + { .compatible = "amazon,al-pcie",
> > + },
> > + {},
> > +};
> > +
> > +static struct platform_driver al_pcie_driver = {
> > + .driver = {
> > + .name = "al-pcie",
> > + .of_match_table = al_pcie_of_match,
> > + .suppress_bind_attrs = true,
> > + },
> > + .probe = al_pcie_probe,
> > +};
> > +builtin_platform_driver(al_pcie_driver);
> > +
> > +#endif /* CONFIG_PCIE_AL*/
> > --
> > 2.17.1
>
>
^ permalink raw reply
* Re: [GIT PULL] Devicetree fixes for 5.3-rc
From: pr-tracker-bot @ 2019-07-21 17:35 UTC (permalink / raw)
To: Rob Herring
Cc: Linus Torvalds, devicetree, linux-kernel@vger.kernel.org,
Frank Rowand
In-Reply-To: <CAL_JsqLTAAT1hEPkxD=BUvmovvA+rdsZdbVfQM6=1m9bvaEysQ@mail.gmail.com>
The pull request you sent on Sat, 20 Jul 2019 20:37:56 -0600:
> git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git tags/devicetree-fixes-for-5.3
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/c7bf0a0f3703bc145368b9ced02749bf75fc718d
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* [PATCH] iio: potentiometer: add a driver for Maxim 5432-5435
From: Martin Kaiser @ 2019-07-21 17:59 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Rob Herring
Cc: linux-iio, devicetree, linux-kernel, Martin Kaiser
Add a driver for the Maxim Integrated MAX5432-MAX5435 family of digital
potentiometers.
These potentiometers are connected via I2C and have 32 wiper positions.
Supported functionality
- set the volatile wiper position
- read the potentiometer scale
Datasheet:
https://datasheets.maximintegrated.com/en/ds/MAX5432-MAX5435.pdf
Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
.../bindings/iio/potentiometer/max5432.txt | 22 ++++
drivers/iio/potentiometer/Kconfig | 11 ++
drivers/iio/potentiometer/Makefile | 1 +
drivers/iio/potentiometer/max5432.c | 131 +++++++++++++++++++++
4 files changed, 165 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/max5432.txt
create mode 100644 drivers/iio/potentiometer/max5432.c
diff --git a/Documentation/devicetree/bindings/iio/potentiometer/max5432.txt b/Documentation/devicetree/bindings/iio/potentiometer/max5432.txt
new file mode 100644
index 000000000000..89ccc8fc7df1
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiometer/max5432.txt
@@ -0,0 +1,22 @@
+* Maxim Integrated MAX5432-MAX5435 Digital Potentiometers.
+
+The node for this driver must be a child node of an I2C controller, hence
+all mandatory properties for your controller must be specified. See directory:
+
+ Documentation/devicetree/bindings/i2c
+
+for more details.
+
+Required properties:
+ - compatible: Must be one of the following, depending on the
+ model:
+ "maxim,max5432"
+ "maxim,max5433"
+ "maxim,max5434"
+ "maxim,max5435"
+
+Example:
+max5434@28 {
+ compatible = "maxim,max5434";
+ reg = <0x28>;
+};
diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index ebc7c72a5e36..4cac0173db8b 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -26,6 +26,17 @@ config DS1803
To compile this driver as a module, choose M here: the
module will be called ds1803.
+config MAX5432
+ tristate "Maxim MAX5432-MAX5435 Digital Potentiometer driver"
+ depends on I2C
+ help
+ Say yes here to build support for the Maxim
+ MAX5432, MAX5433, MAX5434 and MAX5435 digital
+ potentiometer chips.
+
+ To compile this driver as a module, choose M here: the
+ module will be called max5432.
+
config MAX5481
tristate "Maxim MAX5481-MAX5484 Digital Potentiometer driver"
depends on SPI
diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
index 8ff55138cf12..091adf3cdd0b 100644
--- a/drivers/iio/potentiometer/Makefile
+++ b/drivers/iio/potentiometer/Makefile
@@ -6,6 +6,7 @@
# When adding new entries keep the list in alphabetical order
obj-$(CONFIG_AD5272) += ad5272.o
obj-$(CONFIG_DS1803) += ds1803.o
+obj-$(CONFIG_MAX5432) += max5432.o
obj-$(CONFIG_MAX5481) += max5481.o
obj-$(CONFIG_MAX5487) += max5487.o
obj-$(CONFIG_MCP4018) += mcp4018.o
diff --git a/drivers/iio/potentiometer/max5432.c b/drivers/iio/potentiometer/max5432.c
new file mode 100644
index 000000000000..959883297fb5
--- /dev/null
+++ b/drivers/iio/potentiometer/max5432.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Maxim Integrated MAX5432-MAX5435 digital potentiometer driver
+ * Copyright (C) 2019 Martin Kaiser <martin@kaiser.cx>
+ *
+ * Datasheet:
+ * https://datasheets.maximintegrated.com/en/ds/MAX5432-MAX5435.pdf
+ */
+
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+
+/* All chip variants have 32 wiper positions. */
+#define MAX5432_MAX_POS 31
+
+#define OHM_50K (50 * 1000)
+#define OHM_100K (100 * 1000)
+
+/* Update the volatile (currently active) setting. */
+#define CMD_VREG 0x11
+
+struct max5432_data {
+ struct i2c_client *client;
+ u32 ohm;
+};
+
+static const struct iio_chan_spec max5432_channels[] = {
+ {
+ .type = IIO_RESISTANCE,
+ .indexed = 1,
+ .output = 1,
+ .channel = 0,
+ .address = CMD_VREG,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+ }
+};
+
+static int max5432_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct max5432_data *data = iio_priv(indio_dev);
+
+ if (mask != IIO_CHAN_INFO_SCALE)
+ return -EINVAL;
+
+ *val = data->ohm;
+ *val2 = MAX5432_MAX_POS;
+
+ return IIO_VAL_FRACTIONAL;
+}
+
+static int max5432_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct max5432_data *data = iio_priv(indio_dev);
+ u8 data_byte;
+
+ if (mask != IIO_CHAN_INFO_RAW)
+ return -EINVAL;
+
+ if (val < 0 || val > MAX5432_MAX_POS)
+ return -EINVAL;
+
+ if (val2 != 0)
+ return -EINVAL;
+
+ /* Wiper position is in bits D7-D3. (D2-D0 are don't care bits.) */
+ data_byte = val << 3;
+ return i2c_smbus_write_byte_data(
+ data->client, chan->address, data_byte);
+}
+
+static const struct iio_info max5432_info = {
+ .read_raw = max5432_read_raw,
+ .write_raw = max5432_write_raw,
+};
+
+static int max5432_probe(
+ struct i2c_client *client, const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct iio_dev *indio_dev;
+ struct max5432_data *data;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(struct max5432_data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, indio_dev);
+
+ data = iio_priv(indio_dev);
+ data->client = client;
+ data->ohm = (u32)of_device_get_match_data(dev);
+
+ indio_dev->dev.parent = dev;
+ indio_dev->info = &max5432_info;
+ indio_dev->channels = max5432_channels;
+ indio_dev->num_channels = ARRAY_SIZE(max5432_channels);
+ indio_dev->name = client->name;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id max5432_dt_ids[] = {
+ { .compatible = "maxim,max5432", .data = (void *)OHM_50K },
+ { .compatible = "maxim,max5433", .data = (void *)OHM_100K },
+ { .compatible = "maxim,max5434", .data = (void *)OHM_50K },
+ { .compatible = "maxim,max5435", .data = (void *)OHM_100K },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, max5432_dt_ids);
+
+static struct i2c_driver max5432_driver = {
+ .driver = {
+ .name = "max5432",
+ .of_match_table = of_match_ptr(max5432_dt_ids),
+ },
+ .probe = max5432_probe,
+};
+
+module_i2c_driver(max5432_driver);
+
+MODULE_AUTHOR("Martin Kaiser <martin@kaiser.cx>");
+MODULE_DESCRIPTION("max5432-max5435 digital potentiometers");
+MODULE_LICENSE("GPL v2");
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 1/2] dt-bindings: interconnect: Update Qualcomm SDM845 DT bindings
From: Bjorn Andersson @ 2019-07-21 19:10 UTC (permalink / raw)
To: David Dai
Cc: georgi.djakov, robh+dt, evgreen, ilina, seansw, elder,
linux-kernel, devicetree, linux-arm-msm, linux-pm
In-Reply-To: <1563568344-1274-2-git-send-email-daidavid1@codeaurora.org>
On Fri 19 Jul 13:32 PDT 2019, David Dai wrote:
> Redefine the Network-on-Chip devices to more accurately describe
> the interconnect topology on Qualcomm's SDM845 platform. Each
> interconnect device can communicate with different instances of the
> RPMh hardware which are described as RSCs(Resource State Coordinators).
>
> Signed-off-by: David Dai <daidavid1@codeaurora.org>
I like this and we don't have any consumers in DT yet, so I think this
is good.
But we need a patch to the implementation as well, to have the
provider(s) registered with the new compatibles.
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> .../bindings/interconnect/qcom,bcm-voter.txt | 32 +++++++++++++++++
> .../bindings/interconnect/qcom,sdm845.txt | 40 +++++++++++++++++-----
> 2 files changed, 63 insertions(+), 9 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,bcm-voter.txt
>
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,bcm-voter.txt b/Documentation/devicetree/bindings/interconnect/qcom,bcm-voter.txt
> new file mode 100644
> index 0000000..2cf7da2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,bcm-voter.txt
> @@ -0,0 +1,32 @@
> +Qualcomm BCM-Voter interconnect driver binding
> +-----------------------------------------------------------
> +
> +The Bus Clock Manager (BCM) is a dedicated hardware accelerator
> +that manages shared system resources by aggregating requests
> +from multiple Resource State Coordinators (RSC). Interconnect
> +providers are able to vote for aggregated thresholds values from
> +consumers by communicating through their respective RSCs.
> +
> +Required properties :
> +- compatible : shall contain only one of the following:
> + "qcom,sdm845-bcm-voter",
> +
> +Examples:
> +
> +apps_rsc: rsc@179c0000 {
> + label = "apps_rsc";
> + compatible = "qcom,rpmh-rsc";
> +
> + apps_bcm_voter: bcm_voter {
> + compatible = "qcom,sdm845-bcm-voter";
> + };
> +}
> +
> +disp_rsc: rsc@179d0000 {
> + label = "disp_rsc";
> + compatible = "qcom,rpmh-rsc";
> +
> + disp_bcm_voter: bcm_voter {
> + compatible = "qcom,sdm845-bcm-voter";
> + };
> +}
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sdm845.txt b/Documentation/devicetree/bindings/interconnect/qcom,sdm845.txt
> index 5c4f1d9..27f9ed9 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,sdm845.txt
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,sdm845.txt
> @@ -4,21 +4,43 @@ Qualcomm SDM845 Network-On-Chip interconnect driver binding
> SDM845 interconnect providers support system bandwidth requirements through
> RPMh hardware accelerators known as Bus Clock Manager (BCM). The provider is
> able to communicate with the BCM through the Resource State Coordinator (RSC)
> -associated with each execution environment. Provider nodes must reside within
> -an RPMh device node pertaining to their RSC and each provider maps to a single
> -RPMh resource.
> +associated with each execution environment. Provider nodes must point to at
> +least one RPMh device child node pertaining to their RSC and each provider
> +can map to multiple RPMh resources.
>
> Required properties :
> - compatible : shall contain only one of the following:
> - "qcom,sdm845-rsc-hlos"
> + "qcom,sdm845-aggre1_noc",
> + "qcom,sdm845-aggre2_noc",
> + "qcom,sdm845-config_noc",
> + "qcom,sdm845-dc_noc",
> + "qcom,sdm845-gladiator_noc",
> + "qcom,sdm845-mem_noc",
> + "qcom,sdm845-mmss_noc",
> + "qcom,sdm845-system_noc",
> - #interconnect-cells : should contain 1
> +- reg : shall contain base register location and length
> +- qcom,bcm-voter : shall contain phandles to bcm voters
>
> Examples:
>
> -apps_rsc: rsc {
> - rsc_hlos: interconnect {
> - compatible = "qcom,sdm845-rsc-hlos";
> - #interconnect-cells = <1>;
> - };
> +aggre1_noc: interconnect@16e0000 {
> + compatible = "qcom,sdm845-aggre1_noc";
> + reg = <0x16e0000 0xd080>;
> + interconnect-cells = <1>;
> + qcom,bcm-voter = <&apps_bcm_voter>;
> };
>
> +mmss_noc: interconnect@1740000 {
> + compatible = "qcom,sdm845-mmss_noc";
> + reg = <0x1740000 0x1c1000>;
> + interconnect-cells = <1>;
> + qcom,bcm-voter = <&apps_bcm_voter>, <&disp_bcm_voter>;
> +};
> +
> +mem_noc: interconnect@1380000 {
> + compatible = "qcom,sdm845-mem_noc";
> + reg = <0 0x1380000 0 0x27200>;
> + #interconnect-cells = <1>;
> + qcom,bcm-voter = <&apps_bcm_voter>, <&disp_bcm_voter>;
> +};
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: sdm845: Redefine interconnect provider DT nodes
From: Bjorn Andersson @ 2019-07-21 19:13 UTC (permalink / raw)
To: David Dai
Cc: georgi.djakov, robh+dt, evgreen, ilina, seansw, elder,
linux-kernel, devicetree, linux-arm-msm, linux-pm
In-Reply-To: <1563568344-1274-3-git-send-email-daidavid1@codeaurora.org>
On Fri 19 Jul 13:32 PDT 2019, David Dai wrote:
> Add the DT nodes for each of the Network-On-Chip interconnect
> buses found on SDM845 based platform and redefine the rsc_hlos
> child node as a bcm-voter device to better represent the hardware.
>
> Signed-off-by: David Dai <daidavid1@codeaurora.org>
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 61 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index e7d78bc..204222e 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -610,6 +610,62 @@
> #power-domain-cells = <1>;
> };
>
> + aggre1_noc: interconnect@16e0000 {
> + compatible = "qcom,sdm845-aggre1_noc";
> + reg = <0 0x16e0000 0 0xd080>;
Please pad the address to 8 digits and keep nodes sorted by address.
> + #interconnect-cells = <1>;
> + qcom,bcm-voter = <&apps_bcm_voter>;
> + };
> +
> + aggre2_noc: interconnect@1700000 {
> + compatible = "qcom,sdm845-aggre2_noc";
> + reg = <0 0x1700000 0 0x3b100>;
> + #interconnect-cells = <1>;
> + qcom,bcm-voter = <&apps_bcm_voter>;
> + };
> +
> + config_noc: interconnect@1500000 {
> + compatible = "qcom,sdm845-config_noc";
> + reg = <0 0x1500000 0 0x5080>;
> + #interconnect-cells = <1>;
> + qcom,bcm-voter = <&apps_bcm_voter>;
> + };
[..]
> qfprom@784000 {
> compatible = "qcom,qfprom";
> reg = <0 0x00784000 0 0x8ff>;
> @@ -2801,9 +2857,8 @@
> };
> };
>
> - rsc_hlos: interconnect {
> - compatible = "qcom,sdm845-rsc-hlos";
> - #interconnect-cells = <1>;
> + apps_bcm_voter: bcm_voter {
No '_' in node names, so bcm-voter.
Apart from this nits this looks good.
Regards,
Bjorn
> + compatible = "qcom,sdm845-bcm-voter";
> };
> };
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
^ permalink raw reply
* [PATCH V6 00/21] SC7 entry and exit support for Tegra210
From: Sowjanya Komatineni @ 2019-07-21 19:40 UTC (permalink / raw)
To: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, skomatineni, linux-tegra, linux-kernel,
mperttunen, spatra, robh+dt, digetx, devicetree
This patch series includes Tegra210 deepsleep support with RTC alarm
wake event.
This series also includes save and restore of PLLs, clocks, OSC contexts
for deepsleep exit to normal operation.
This patch series doesn't support 100% suspend/resume to allow fully
functional state upon resume and we are working on some more drivers suspend
and resume implementations.
[V6]: Changes between V5 & V6 are
- V5 feedback fixes
- DFLL suspend and resume moved to DFLL clock driver
- Add suspend and resume support for CPUFreq driver to explicitly
switch source to safe source of PLLP and disable DFLL clock.
- Fix to super clock driver to enable PLLP branch to CPU before
source switch to PLLP.
- Added save and restore support for super clock driver.
[V5]: Changes between V4 & V5 are
- V4 feedback fixes
[V4]: Changes between V3 & V4 are
- V3 feedback fixes
- Removed park bits clear for EMMC pads in pinctrl-tegra driver
function tegra_pinctrl_clear_parked_bits as based on V3 feedback
parked_bit is updated to parked_bitmask to use with DRV_PINGROUP
as well and thierry posted patch series for this.
- Implemented all peripheral clocks save and restore through their
corresponding clk_ops save_context and restore_context and removed
all direct registers store and restore in clk-tegra210 driver.
- Created separate patch for fence_delay update during PLLU init based
on V3 feedback.
- Added more comments in tegra210_clk_resume regarding dfll restore
sequence and its dependency on peripheral clocks restore.
[V3]: Changes between V2 & V3 are
- V2 feedback fixes
- GPIO restore should happen prior to Pinctrl restore to prevent
glitch on GPIO lines. So using resume_noirq for gpio tegra to allow
gpio resume prior to pinctrl resume.
- Implemented save_context and restore_context callbacks for clock
plls, pll outs and dividers in corresponding drivers.
Note: Peripheral clocks and clock enable and reset need to be in
Tegra210 clock suspend/resume as they need to be in proper sequence
w.r.t DFLL resume for restoring CPU clock.
- Removed gpio-tegra changes for hierarchical support to have PMC as
parent to GPIOs for GPIO wake event support. Thierry is working on
gpiolib for some cleanup before adding hierarchical support. So
holding on to GPIO wake support for now.
[V2] : V1 feedback fixes
Patch 0002: This version still using syscore. Thierry suggest not to
use syscore and waiting on suggestion from Linux Walleij for any better
way of storing current state of pins before suspend entry and restoring
them on resume at very early stage. So left this the same way as V1 and
will address once I get more feedback on this.
Also need to findout and implement proper way of forcing resume order
between pinctrl and gpio driver.
[V1]: Tegra210 SC7 entry and exit thru RTC wake and Power button GPIO wake
using hierarchical IRQ with PMC as parent to GPIO.
Sowjanya Komatineni (21):
irqchip: tegra: Do not disable COP IRQ during suspend
pinctrl: tegra: Add suspend and resume support
pinctrl: tegra210: Add Tegra210 pinctrl pm ops
clk: tegra: Save and restore divider rate
clk: tegra: pllout: Save and restore pllout context
clk: tegra: pll: Save and restore pll context
clk: tegra: Support for OSC context save and restore
clk: tegra: clk-periph: Add save and restore support
clk: tegra: clk-super: Fix to enable PLLP branches to CPU
clk: tegra: clk-super: Add save and restore support
clk: tegra: clk-dfll: Add suspend and resume support
cpufreq: tegra124: Add suspend and resume support
clk: tegra210: Use fence_udelay during PLLU init
clk: tegra210: Add suspend and resume support
soc/tegra: pmc: Allow to support more tegras wake
soc/tegra: pmc: Add pmc wake support for tegra210
arm64: tegra: Enable wake from deep sleep on RTC alarm.
soc/tegra: pmc: Configure core power request polarity
soc/tegra: pmc: Configure deep sleep control settings
arm64: dts: tegra210-p2180: Jetson TX1 SC7 timings
arm64: dts: tegra210-p3450: Jetson nano SC7 timings
arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi | 7 +
arch/arm64/boot/dts/nvidia/tegra210-p3450-0000.dts | 7 +
arch/arm64/boot/dts/nvidia/tegra210.dtsi | 5 +-
drivers/clk/tegra/clk-dfll.c | 44 +++++++
drivers/clk/tegra/clk-dfll.h | 2 +
drivers/clk/tegra/clk-divider.c | 23 ++++
drivers/clk/tegra/clk-periph-fixed.c | 33 +++++
drivers/clk/tegra/clk-periph-gate.c | 34 +++++
drivers/clk/tegra/clk-periph.c | 43 +++++++
drivers/clk/tegra/clk-pll-out.c | 28 ++++
drivers/clk/tegra/clk-pll.c | 121 +++++++++++++-----
drivers/clk/tegra/clk-sdmmc-mux.c | 30 +++++
drivers/clk/tegra/clk-super.c | 51 ++++++++
drivers/clk/tegra/clk-tegra-fixed.c | 15 +++
drivers/clk/tegra/clk-tegra-super-gen4.c | 4 +-
drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 1 +
drivers/clk/tegra/clk-tegra210.c | 81 ++++++++++--
drivers/clk/tegra/clk.c | 14 ++
drivers/clk/tegra/clk.h | 36 +++++-
drivers/cpufreq/tegra124-cpufreq.c | 46 +++++++
drivers/irqchip/irq-tegra.c | 20 ++-
drivers/pinctrl/tegra/pinctrl-tegra.c | 59 +++++++++
drivers/pinctrl/tegra/pinctrl-tegra.h | 3 +
drivers/pinctrl/tegra/pinctrl-tegra210.c | 1 +
drivers/soc/tegra/pmc.c | 142 ++++++++++++++++++++-
25 files changed, 798 insertions(+), 52 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH V6 01/21] irqchip: tegra: Do not disable COP IRQ during suspend
From: Sowjanya Komatineni @ 2019-07-21 19:40 UTC (permalink / raw)
To: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, skomatineni, linux-tegra, linux-kernel,
mperttunen, spatra, robh+dt, digetx, devicetree
In-Reply-To: <1563738060-30213-1-git-send-email-skomatineni@nvidia.com>
Tegra210 platforms use sc7 entry firmware to program Tegra LP0/SC7 entry
sequence and sc7 entry firmware is run from COP/BPMP-Lite.
So, COP/BPMP-Lite still need IRQ function to finish SC7 suspend sequence
for Tegra210.
This patch has fix for leaving the COP IRQ enabled for Tegra210 during
interrupt controller suspend operation.
Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
index e1f771c72fc4..851f88cef508 100644
--- a/drivers/irqchip/irq-tegra.c
+++ b/drivers/irqchip/irq-tegra.c
@@ -44,6 +44,7 @@ static unsigned int num_ictlrs;
struct tegra_ictlr_soc {
unsigned int num_ictlrs;
+ bool supports_sc7;
};
static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
@@ -56,6 +57,7 @@ static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
.num_ictlrs = 6,
+ .supports_sc7 = true,
};
static const struct of_device_id ictlr_matches[] = {
@@ -67,6 +69,7 @@ static const struct of_device_id ictlr_matches[] = {
struct tegra_ictlr_info {
void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
+ const struct tegra_ictlr_soc *soc;
#ifdef CONFIG_PM_SLEEP
u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
@@ -147,8 +150,20 @@ static int tegra_ictlr_suspend(void)
lic->cop_ier[i] = readl_relaxed(ictlr + ICTLR_COP_IER);
lic->cop_iep[i] = readl_relaxed(ictlr + ICTLR_COP_IEP_CLASS);
- /* Disable COP interrupts */
- writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
+ /*
+ * AVP/COP/BPMP-Lite is the Tegra boot processor.
+ *
+ * Tegra210 system suspend flow uses sc7entry firmware which
+ * is executed by COP/BPMP and it includes disabling COP IRQ,
+ * clamping CPU rail, turning off VDD_CPU, and preparing the
+ * system to go to SC7/LP0.
+ *
+ * COP/BPMP wakes up when COP IRQ is triggered and runs
+ * sc7entry-firmware. So need to keep COP interrupt enabled.
+ */
+ if (!lic->soc->supports_sc7)
+ /* Disable COP interrupts if SC7 is not supported */
+ writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
/* Disable CPU interrupts */
writel_relaxed(~0ul, ictlr + ICTLR_CPU_IER_CLR);
@@ -339,6 +354,7 @@ static int __init tegra_ictlr_init(struct device_node *node,
goto out_unmap;
}
+ lic->soc = soc;
tegra_ictlr_syscore_init();
pr_info("%pOF: %d interrupts forwarded to %pOF\n",
--
2.7.4
^ permalink raw reply related
* [PATCH V6 02/21] pinctrl: tegra: Add suspend and resume support
From: Sowjanya Komatineni @ 2019-07-21 19:40 UTC (permalink / raw)
To: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, skomatineni, linux-tegra, linux-kernel,
mperttunen, spatra, robh+dt, digetx, devicetree
In-Reply-To: <1563738060-30213-1-git-send-email-skomatineni@nvidia.com>
This patch adds support for Tegra pinctrl driver suspend and resume.
During suspend, context of all pinctrl registers are stored and
on resume they are all restored to have all the pinmux and pad
configuration for normal operation.
Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
drivers/pinctrl/tegra/pinctrl-tegra.c | 59 +++++++++++++++++++++++++++++++++++
drivers/pinctrl/tegra/pinctrl-tegra.h | 3 ++
2 files changed, 62 insertions(+)
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
index 186ef98e7b2b..e3a237534281 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
@@ -631,6 +631,58 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
}
}
+static size_t tegra_pinctrl_get_bank_size(struct device *dev,
+ unsigned int bank_id)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct resource *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, bank_id);
+
+ return resource_size(res) / 4;
+}
+
+static int tegra_pinctrl_suspend(struct device *dev)
+{
+ struct tegra_pmx *pmx = dev_get_drvdata(dev);
+ u32 *backup_regs = pmx->backup_regs;
+ u32 *regs;
+ size_t bank_size;
+ unsigned int i, k;
+
+ for (i = 0; i < pmx->nbanks; i++) {
+ bank_size = tegra_pinctrl_get_bank_size(dev, i);
+ regs = pmx->regs[i];
+ for (k = 0; k < bank_size; k++)
+ *backup_regs++ = readl_relaxed(regs++);
+ }
+
+ return pinctrl_force_sleep(pmx->pctl);
+}
+
+static int tegra_pinctrl_resume(struct device *dev)
+{
+ struct tegra_pmx *pmx = dev_get_drvdata(dev);
+ u32 *backup_regs = pmx->backup_regs;
+ u32 *regs;
+ size_t bank_size;
+ unsigned int i, k;
+
+ for (i = 0; i < pmx->nbanks; i++) {
+ bank_size = tegra_pinctrl_get_bank_size(dev, i);
+ regs = pmx->regs[i];
+ for (k = 0; k < bank_size; k++)
+ writel_relaxed(*backup_regs++, regs++);
+ }
+
+ return 0;
+}
+
+const struct dev_pm_ops tegra_pinctrl_pm = {
+ .suspend = &tegra_pinctrl_suspend,
+ .resume = &tegra_pinctrl_resume
+};
+
static bool gpio_node_has_range(const char *compatible)
{
struct device_node *np;
@@ -655,6 +707,7 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
int i;
const char **group_pins;
int fn, gn, gfn;
+ unsigned long backup_regs_size = 0;
pmx = devm_kzalloc(&pdev->dev, sizeof(*pmx), GFP_KERNEL);
if (!pmx)
@@ -707,6 +760,7 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
res = platform_get_resource(pdev, IORESOURCE_MEM, i);
if (!res)
break;
+ backup_regs_size += resource_size(res);
}
pmx->nbanks = i;
@@ -715,6 +769,11 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
if (!pmx->regs)
return -ENOMEM;
+ pmx->backup_regs = devm_kzalloc(&pdev->dev, backup_regs_size,
+ GFP_KERNEL);
+ if (!pmx->backup_regs)
+ return -ENOMEM;
+
for (i = 0; i < pmx->nbanks; i++) {
res = platform_get_resource(pdev, IORESOURCE_MEM, i);
pmx->regs[i] = devm_ioremap_resource(&pdev->dev, res);
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
index 105309774079..0fc82eea9cf1 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.h
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
@@ -17,6 +17,7 @@ struct tegra_pmx {
int nbanks;
void __iomem **regs;
+ u32 *backup_regs;
};
enum tegra_pinconf_param {
@@ -193,6 +194,8 @@ struct tegra_pinctrl_soc_data {
bool drvtype_in_mux;
};
+extern const struct dev_pm_ops tegra_pinctrl_pm;
+
int tegra_pinctrl_probe(struct platform_device *pdev,
const struct tegra_pinctrl_soc_data *soc_data);
#endif
--
2.7.4
^ permalink raw reply related
* [PATCH V6 03/21] pinctrl: tegra210: Add Tegra210 pinctrl pm ops
From: Sowjanya Komatineni @ 2019-07-21 19:40 UTC (permalink / raw)
To: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, skomatineni, linux-tegra, linux-kernel,
mperttunen, spatra, robh+dt, digetx, devicetree
In-Reply-To: <1563738060-30213-1-git-send-email-skomatineni@nvidia.com>
This patch adds suspend and resume functionality to Tegra210 pinctrl.
Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
drivers/pinctrl/tegra/pinctrl-tegra210.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c b/drivers/pinctrl/tegra/pinctrl-tegra210.c
index 39ab6480a941..fc072a36deb3 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
@@ -1571,6 +1571,7 @@ static struct platform_driver tegra210_pinctrl_driver = {
.driver = {
.name = "tegra210-pinctrl",
.of_match_table = tegra210_pinctrl_of_match,
+ .pm = &tegra_pinctrl_pm,
},
.probe = tegra210_pinctrl_probe,
};
--
2.7.4
^ permalink raw reply related
* [PATCH V6 04/21] clk: tegra: Save and restore divider rate
From: Sowjanya Komatineni @ 2019-07-21 19:40 UTC (permalink / raw)
To: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, skomatineni, linux-tegra, linux-kernel,
mperttunen, spatra, robh+dt, digetx, devicetree
In-Reply-To: <1563738060-30213-1-git-send-email-skomatineni@nvidia.com>
This patch implements context save and restore for clock divider.
During system suspend, core power goes off and looses the settings
of the Tegra CAR controller registers.
So during suspend entry the context of clock divider is saved and
on resume context is restored back for normal operation.
Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
drivers/clk/tegra/clk-divider.c | 23 +++++++++++++++++++++++
drivers/clk/tegra/clk.h | 2 ++
2 files changed, 25 insertions(+)
diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
index e76731fb7d69..ecb7ff9ce97e 100644
--- a/drivers/clk/tegra/clk-divider.c
+++ b/drivers/clk/tegra/clk-divider.c
@@ -109,10 +109,33 @@ static int clk_frac_div_set_rate(struct clk_hw *hw, unsigned long rate,
return 0;
}
+static int clk_divider_save_context(struct clk_hw *hw)
+{
+ struct tegra_clk_frac_div *divider = to_clk_frac_div(hw);
+ struct clk_hw *parent = clk_hw_get_parent(hw);
+ unsigned long parent_rate = clk_hw_get_rate(parent);
+
+ divider->rate = clk_frac_div_recalc_rate(hw, parent_rate);
+
+ return 0;
+}
+
+static void clk_divider_restore_context(struct clk_hw *hw)
+{
+ struct tegra_clk_frac_div *divider = to_clk_frac_div(hw);
+ struct clk_hw *parent = clk_hw_get_parent(hw);
+ unsigned long parent_rate = clk_hw_get_rate(parent);
+
+ if (clk_frac_div_set_rate(hw, divider->rate, parent_rate) < 0)
+ WARN_ON(1);
+}
+
const struct clk_ops tegra_clk_frac_div_ops = {
.recalc_rate = clk_frac_div_recalc_rate,
.set_rate = clk_frac_div_set_rate,
.round_rate = clk_frac_div_round_rate,
+ .save_context = clk_divider_save_context,
+ .restore_context = clk_divider_restore_context,
};
struct clk *tegra_clk_register_divider(const char *name,
diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
index 905bf1096558..83623f5f55f3 100644
--- a/drivers/clk/tegra/clk.h
+++ b/drivers/clk/tegra/clk.h
@@ -42,6 +42,7 @@ struct clk *tegra_clk_register_sync_source(const char *name,
* @width: width of the divider bit field
* @frac_width: width of the fractional bit field
* @lock: register lock
+ * @rate: rate during suspend and resume
*
* Flags:
* TEGRA_DIVIDER_ROUND_UP - This flags indicates to round up the divider value.
@@ -62,6 +63,7 @@ struct tegra_clk_frac_div {
u8 width;
u8 frac_width;
spinlock_t *lock;
+ unsigned long rate;
};
#define to_clk_frac_div(_hw) container_of(_hw, struct tegra_clk_frac_div, hw)
--
2.7.4
^ permalink raw reply related
* [PATCH V6 05/21] clk: tegra: pllout: Save and restore pllout context
From: Sowjanya Komatineni @ 2019-07-21 19:40 UTC (permalink / raw)
To: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, skomatineni, linux-tegra, linux-kernel,
mperttunen, spatra, robh+dt, digetx, devicetree
In-Reply-To: <1563738060-30213-1-git-send-email-skomatineni@nvidia.com>
This patch implements save and restore of pllout context.
During system suspend, core power goes off and looses the settings
of the Tegra CAR controller registers.
So during suspend entry the state of pllout is saved and on resume
it is restored back to have pllout in same state as before suspend.
pllout rate is saved and restore in clock divider so it will be at
same rate as before suspend when pllout state is restored.
Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
drivers/clk/tegra/clk-pll-out.c | 28 ++++++++++++++++++++++++++++
drivers/clk/tegra/clk-tegra210.c | 3 ++-
drivers/clk/tegra/clk.h | 9 +++++++++
3 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/tegra/clk-pll-out.c b/drivers/clk/tegra/clk-pll-out.c
index 35f2bf00e1e6..8f26a7e3e579 100644
--- a/drivers/clk/tegra/clk-pll-out.c
+++ b/drivers/clk/tegra/clk-pll-out.c
@@ -69,10 +69,38 @@ static void clk_pll_out_disable(struct clk_hw *hw)
spin_unlock_irqrestore(pll_out->lock, flags);
}
+static int tegra_clk_pll_out_save_context(struct clk_hw *hw)
+{
+ struct tegra_clk_pll_out *pll_out = to_clk_pll_out(hw);
+
+ if (pll_out->flags & TEGRA_PLLRE_OUT)
+ pll_out->pllout_ctx = readl_relaxed(pll_out->reg);
+ else
+ pll_out->pllout_ctx = clk_hw_get_rate(hw);
+
+ return 0;
+}
+
+static void tegra_clk_pll_out_restore_context(struct clk_hw *hw)
+{
+ struct tegra_clk_pll_out *pll_out = to_clk_pll_out(hw);
+
+ if (pll_out->flags & TEGRA_PLLRE_OUT) {
+ writel_relaxed(pll_out->pllout_ctx, pll_out->reg);
+ } else {
+ if (!__clk_get_enable_count(hw->clk))
+ clk_pll_out_disable(hw);
+ else
+ clk_pll_out_enable(hw);
+ }
+}
+
const struct clk_ops tegra_clk_pll_out_ops = {
.is_enabled = clk_pll_out_is_enabled,
.enable = clk_pll_out_enable,
.disable = clk_pll_out_disable,
+ .save_context = tegra_clk_pll_out_save_context,
+ .restore_context = tegra_clk_pll_out_restore_context,
};
struct clk *tegra_clk_register_pll_out(const char *name,
diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
index df172d5772d7..4721ee030d1c 100644
--- a/drivers/clk/tegra/clk-tegra210.c
+++ b/drivers/clk/tegra/clk-tegra210.c
@@ -3200,7 +3200,8 @@ static void __init tegra210_pll_init(void __iomem *clk_base,
8, 8, 1, NULL);
clk = tegra_clk_register_pll_out("pll_re_out1", "pll_re_out1_div",
clk_base + PLLRE_OUT1, 1, 0,
- CLK_SET_RATE_PARENT, 0, NULL);
+ CLK_SET_RATE_PARENT, TEGRA_PLLRE_OUT,
+ NULL);
clks[TEGRA210_CLK_PLL_RE_OUT1] = clk;
/* PLLE */
diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
index 83623f5f55f3..fb29a8c27873 100644
--- a/drivers/clk/tegra/clk.h
+++ b/drivers/clk/tegra/clk.h
@@ -439,6 +439,12 @@ struct clk *tegra_clk_register_pllu_tegra210(const char *name,
* @rst_bit_idx: bit to reset PLL divider
* @lock: register lock
* @flags: hardware-specific flags
+ * @pllout_ctx: pllout context to save and restore during suspend
+ * and resume
+ *
+ * Flags:
+ * TEGRA_PLLRE_OUT - This flag indicates that it is PLLRE_OUT and is used to
+ * identify PLLRE_OUT during clk_pll_out save and restore.
*/
struct tegra_clk_pll_out {
struct clk_hw hw;
@@ -447,8 +453,11 @@ struct tegra_clk_pll_out {
u8 rst_bit_idx;
spinlock_t *lock;
u8 flags;
+ unsigned int pllout_ctx;
};
+#define TEGRA_PLLRE_OUT BIT(0)
+
#define to_clk_pll_out(_hw) container_of(_hw, struct tegra_clk_pll_out, hw)
extern const struct clk_ops tegra_clk_pll_out_ops;
--
2.7.4
^ permalink raw reply related
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