* Re: [PATCH v2 13/15] arm64: dts: imx: add imx8qm common dts file
From: Dong Aisheng @ 2019-07-17 8:28 UTC (permalink / raw)
To: Fabio Estevam
Cc: Dong Aisheng,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Catalin Marinas, Will Deacon, Rob Herring, NXP Linux Team,
Sascha Hauer, Fabio Estevam, Mark Rutland, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAOMZO5Cc6XLjFFEVd59Pru7t72jvW=OCikyXK7W6E8rWRVO3jA@mail.gmail.com>
On Wed, Jul 17, 2019 at 3:33 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Tue, Jul 16, 2019 at 12:37 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
>
> > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > @@ -144,7 +144,7 @@
> > };
> >
> > clk: clock-controller {
> > - compatible = "fsl,imx8qxp-clk";
> > + compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
>
> This looks like unrelated to adding imx8qm support.
Sorry, it's a mistake. Will remove in next version.
I will first wait to see if any more comments on the rest part.
Regards
Aisheng
^ permalink raw reply
* Re: [PATCH v2 10/15] arm64: dts: imx8qm: add conn ss support
From: Dong Aisheng @ 2019-07-17 8:26 UTC (permalink / raw)
To: Fabio Estevam
Cc: Dong Aisheng,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Catalin Marinas, Will Deacon, Rob Herring, NXP Linux Team,
Sascha Hauer, Fabio Estevam, Mark Rutland, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAOMZO5B=yK_S=2xRFt_1pAJ2iYE7CCCenOKQ3rjOeF3e+=vQtQ@mail.gmail.com>
On Wed, Jul 17, 2019 at 3:31 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Tue, Jul 16, 2019 at 12:37 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
> >
> > The CONN SS of MX8QM is mostly the same as MX8QXP except it has one more
> > USB HSIC module support. So we can fully reuse the exist CONN SS dtsi.
> > Add <soc>-ss-conn.dtsi with compatible string updated according to
> > imx8-ss-conn.dtsi.
>
> Subject and commit log does not match what the patch is actually doing.
What 'not match' did you mean?
Regards
Aisheng
^ permalink raw reply
* Re: [PATCH v2 09/15] arm64: dts: imx8qm: add lsio ss support
From: Dong Aisheng @ 2019-07-17 8:24 UTC (permalink / raw)
To: Fabio Estevam
Cc: Dong Aisheng,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Catalin Marinas, Will Deacon, Rob Herring, NXP Linux Team,
Sascha Hauer, Fabio Estevam, Mark Rutland, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAOMZO5DZDX4hGmLLT0+5e0VDhws-DknhAwyKyAxva8x5F2BYjw@mail.gmail.com>
On Wed, Jul 17, 2019 at 3:30 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Tue, Jul 16, 2019 at 12:37 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
>
> > +&lsio_gpio0 {
> > + compatible = "fsl,imx8qm-gpio", "fsl,imx35-gpio";
>
> fsl,imx8qm-gpio needs to be documented.
Yes, as documented in cover-letter.
Will submit in a separate patch.
Regards
Aisheng
^ permalink raw reply
* Re: [PATCH v2 07/15] arm64: dts: imx8: switch to two cell scu clock binding
From: Dong Aisheng @ 2019-07-17 8:23 UTC (permalink / raw)
To: Fabio Estevam
Cc: Dong Aisheng,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Catalin Marinas, Will Deacon, Rob Herring, NXP Linux Team,
Sascha Hauer, Fabio Estevam, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAOMZO5BMxZVuwz0jJ410H9Ey4iWD67mCdQhhrtnQ182DbcgtBg@mail.gmail.com>
On Wed, Jul 17, 2019 at 3:29 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Tue, Jul 16, 2019 at 12:37 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
> >
> > switch to two cell scu clock binding
>
> Please explain why you are doing this.
Because we want to eliminate the using of SW Clock IDs.
For more details, please refer to:
https://patchwork.kernel.org/cover/11046287/
Regards
Aisheng
^ permalink raw reply
* Re: [PATCH v2 06/15] arm64: dts: imx8: add adma lpcg clocks
From: Dong Aisheng @ 2019-07-17 8:22 UTC (permalink / raw)
To: Fabio Estevam
Cc: Dong Aisheng,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Catalin Marinas, Will Deacon, Rob Herring, NXP Linux Team,
Sascha Hauer, Fabio Estevam, Mark Rutland, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAOMZO5CdBhJmdbavfWzpx0YPhAXgQvA+ik3GMz0e5U1j6ytyHg@mail.gmail.com>
On Wed, Jul 17, 2019 at 3:28 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Tue, Jul 16, 2019 at 12:37 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
>
> > + uart0_lpcg: clock-controller@5a460000 {
> > + reg = <0x5a460000 0x10000>;
> > + #clock-cells = <1>;
> > + clocks = <&clk IMX_ADMA_UART0_CLK>,
> > + <&dma_ipg_clk>;
>
> Putting the clocks into a single line helps readability, even if it is
> over 80 col.
>
Is this a new rule? I'm not aware of this requirement.
By looking at imx8mq.dts, it seems also have not followed that rule.
I'm a bit concern that it might be super long for other lpcgs with 5 clocks.
e.g. enet.
Regards
Aisheng
> > + bit-offset = <0 16>;
> > + clock-output-names = "uart0_lpcg_baud_clk",
> > + "uart0_lpcg_ipg_clk";
>
> Same here.
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx6ul-kontron-ul2: Add Exceet/Kontron iMX6-UL2 SoM
From: Krzysztof Kozlowski @ 2019-07-17 8:17 UTC (permalink / raw)
To: Schrempf Frieder
Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <cde6f251-4b15-a4f0-57ed-ca2ed014b511@kontron.de>
On Tue, 16 Jul 2019 at 17:38, Schrempf Frieder
<frieder.schrempf@kontron.de> wrote:
>
> Hi Krzysztof,
>
> On 12.07.19 16:12, Krzysztof Kozlowski wrote:
> > Add support for iMX6-UL2 modules from Kontron Electronics GmbH (before
> > acquisition: Exceet Electronics) and evalkit boards based on it:
> >
> > 1. i.MX6 UL System-on-Module, a 25x25 mm solderable module (LGA pads and
> > pin castellations) with 256 MB RAM, 1 MB NOR-Flash, 256 MB NAND and
> > other interfaces,
> > 1. UL2 evalkit, w/wo eMMC, without display,
> > 2. UL2 evalkit with 4.3" display,
> > 3. UL2 evalkit with 5.0" display.
>
> We will use a new naming scheme for these and other boards. The new
> names would be:
>
> 1. Kontron N6310 SOM (i.MX6UL SoM with 256MB RAM/NAND)
> 2. Kontron N6310 S (Evalkit with SoM)
> 3. Kontron N6310 S 43 (Evalkit with SoM and 4.3" display)
> 4. Kontron N6310 S 50 (Evalkit with SoM and 5.0" display)
OK (and OK for all other comments which I will skip below).
(...)
> > +
> > + memory@80000000 {
> > + reg = <0x80000000 0x10000000>;
> > + };
> > +};
> > +
> > +&cpu0 {
> > + clock-frequency = <528000000>;
> > + operating-points = <
> > + /* kHz uV */
> > + 528000 1175000
> > + 396000 1025000
> > + 198000 950000
> > + >;
> > + fsl,soc-operating-points = <
> > + /* KHz uV */
> > + 528000 1175000
> > + 396000 1175000
> > + 198000 1175000
> > + >;
> > +};
>
> What's the reason behind overwriting the operating-points and setting
> clock-frequency? Doesn't imx6q-cpufreq.c already read the speed grades
> from the hardware and adjust the operating-points accordingly?
Good point. From the driver point of view overwriting of opps is not
needed. As you said, the driver will adjust the speed to the reported
grade. This could have meaning for the completeness of hardware
description however I see that there are no even bindings for CPU and
other boards do not overwrite it. I'll skip it then.
(...)
> > +
> > + regulators {
> > + compatible = "simple-bus";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
>
> We copied this from some other devicetree and I'm not sure in what case
> we should really group the regulators in a simple-bus, or what's the
> reason behind this. But others do it like this, so it's probably not so
> wrong.
Either simple-bus with regulator@address or unique regulator node
names (regulator-1, no unit address). Both are popular but I think in
recent submissions and comments Rob Herring was proposing the second
option - unique regulator names without addresses. I can use such
approach (unless DTC complains).
Thanks for review and feedback!
Best regards,
Krzysztof
^ permalink raw reply
* RE: [PATCH v2 15/15] arm64: defconfig: add imx8qm mek support
From: Aisheng Dong @ 2019-07-17 8:16 UTC (permalink / raw)
To: Fabio Estevam
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Dong Aisheng, Catalin Marinas, Will Deacon, Rob Herring,
dl-linux-imx, Sascha Hauer, Fabio Estevam, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAOMZO5CTmAdxRK4FZu+Lw3WxkBzdufNJ=gE11TFSoizdYwXnbw@mail.gmail.com>
> From: Fabio Estevam <festevam@gmail.com>
> Sent: Wednesday, July 17, 2019 3:25 AM
>
> Hi Dong,
>
> On Tue, Jul 16, 2019 at 12:38 PM Dong Aisheng <aisheng.dong@nxp.com>
> wrote:
> >
> > add imx8qm mek support
>
> The Subject and commit log does not match with the patch content.
Because mx8qm shares the most things from mx8qxp which has already
been enabled in defconfig. The only one left is pinctrl driver.
I would update the title a bit to be more specific in next version.
Regards
Aisheng
^ permalink raw reply
* Re: [PATCH v2 02/15] arm64: dts: imx8qxp: move scu pd node before scu clock node
From: Dong Aisheng @ 2019-07-17 8:14 UTC (permalink / raw)
To: Fabio Estevam
Cc: Dong Aisheng,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Catalin Marinas, Will Deacon, Rob Herring, NXP Linux Team,
Sascha Hauer, Fabio Estevam, Mark Rutland, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAOMZO5CMM94rAOryvfmHpuAsJnCRj-98kX3rfiZSn5EZt_c0kA@mail.gmail.com>
On Wed, Jul 17, 2019 at 3:26 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Tue, Jul 16, 2019 at 12:37 PM Dong Aisheng <aisheng.dong@nxp.com> wrote:
> >
> > SCU clock depends on SCU Power domain. So let's move scu pd node
> > before scu clock to make it probe earlier.
>
> This looks like a fragile solution. Shouldn't this be handled via
> probe deferring?
Not like a normal case that there may be only a few devices may get deferred,
for qm/qxp, almost all the devices (~100) may get deferred without
this patch. e.g. scu clk, lpcg clk and all other devices which depend on clk.
So i think it's really unnecessary to do that.
Regards
Aisheng
^ permalink raw reply
* Re: [PATCH v3 1/6] dt-bindings: opp: Introduce opp-peak-KBps and opp-avg-KBps bindings
From: Viresh Kumar @ 2019-07-17 7:54 UTC (permalink / raw)
To: Saravana Kannan
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, vincent.guittot,
seansw, daidavid1, Rajendra Nayak, sibis, bjorn.andersson,
evgreen, kernel-team, linux-pm, devicetree, linux-kernel
In-Reply-To: <20190703011020.151615-2-saravanak@google.com>
On 02-07-19, 18:10, Saravana Kannan wrote:
> Interconnects often quantify their performance points in terms of
> bandwidth. So, add opp-peak-KBps (required) and opp-avg-KBps (optional) to
> allow specifying Bandwidth OPP tables in DT.
>
> opp-peak-KBps is a required property that replace opp-hz for Bandwidth OPP
> tables.
>
> opp-avg-KBps is an optional property that can be used in Bandwidth OPP
> tables.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> Documentation/devicetree/bindings/opp/opp.txt | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index 76b6c79604a5..c869e87caa2a 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -83,9 +83,14 @@ properties.
>
> Required properties:
> - opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
> - required property for all device nodes but devices like power domains. The
> - power domain nodes must have another (implementation dependent) property which
> - uniquely identifies the OPP nodes.
> + required property for all device nodes but for devices like power domains or
> + bandwidth opp tables. The power domain nodes must have another (implementation
> + dependent) property which uniquely identifies the OPP nodes. The interconnect
> + opps are required to have the opp-peak-bw property.
??
> +
> +- opp-peak-KBps: Peak bandwidth in kilobytes per second, expressed as a 32-bit
> + big-endian integer. This is a required property for all devices that don't
> + have opp-hz. For example, bandwidth OPP tables for interconnect paths.
>
> Optional properties:
> - opp-microvolt: voltage in micro Volts.
> @@ -132,6 +137,10 @@ Optional properties:
> - opp-level: A value representing the performance level of the device,
> expressed as a 32-bit integer.
>
> +- opp-avg-KBps: Average bandwidth in kilobytes per second, expressed as a
> + 32-bit big-endian integer. This property is only meaningful in OPP tables
> + where opp-peak-KBps is present.
> +
> - clock-latency-ns: Specifies the maximum possible transition latency (in
> nanoseconds) for switching to this OPP from any other OPP.
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
--
viresh
^ permalink raw reply
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp
From: Alexander Steffen @ 2019-07-17 7:48 UTC (permalink / raw)
To: Tomer Maimon, Oshri Alkobi, Jarkko Sakkinen
Cc: Rob Herring, Mark Rutland, peterhuewe, jgg, Arnd Bergmann,
Greg KH, IS20 Oshri Alkoby, devicetree,
AP MS30 Linux Kernel community, linux-integrity, gcwilson,
kgoldman, nayna, IS30 Dan Morav, eyal.cohen
In-Reply-To: <CAP6Zq1hPo9dG71YFyr7z9rjmi-DvoUZJOme4+2uqsfO+7nH+HQ@mail.gmail.com>
On 15.07.2019 10:08, Tomer Maimon wrote:
> Hi Jarkko and All,
>
> Thanks for your feedback and sorry for the late response.
>
>
> Due to the amount of work required to handle this technical feedback and
> project constraints we need to put this task on hold for the near future.
>
> In the meantime, anyone from the community is welcome to take over this
> code and handle the re-design for the benefit of the entire TPM community.
Too bad you lack the time to finish this.
If somebody else were to pick it up, would it be possible for you to
provide a couple of TPMs that implement this protocol, so that it can be
properly tested?
Alexander
^ permalink raw reply
* Re: [RFC PATCH V3 1/4] media: platform: mtk-isp: Add Mediatek sensor interface driver
From: Tomasz Figa @ 2019-07-17 7:45 UTC (permalink / raw)
To: Louis Kuo
Cc: devicetree, Sean.Cheng, laurent.pinchart+renesas, Rynn.Wu,
christie.yu, srv_heupstream, holmes.chiou, Jerry-ch.Chen,
keiichiw, jungo.lin, sj.huang, yuzhao, hans.verkuil, zwisler,
frederic.chen, matthias.bgg, linux-mediatek, mchehab,
linux-arm-kernel, linux-media
In-Reply-To: <1559815233-24796-2-git-send-email-louis.kuo@mediatek.com>
Hi Louis,
On Thu, Jun 06, 2019 at 06:00:30PM +0800, Louis Kuo wrote:
> This patch adds Mediat:ek's sensor interface driver. Sensor interface
> driver
> is a MIPI-CSI2 host driver, namely, a HW camera interface controller.
> It support a widely adopted, simple, high-speed protocol primarily
> intended
> for point-to-point image and video transmission between cameras and host
> devices.
>
> The mtk-isp directory will contain drivers for multiple IP blocks found in
> Mediatek ISP system. It will include ISP Pass 1 driver, sensor interface
> driver, DIP driver and face detection driver.
>
> Signed-off-by: Louis Kuo <louis.kuo@mediatek.com>
> ---
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/mtk-isp/Makefile | 3 +
> drivers/media/platform/mtk-isp/isp_50/Makefile | 5 +
> .../media/platform/mtk-isp/isp_50/seninf/Makefile | 6 +
> .../platform/mtk-isp/isp_50/seninf/mtk_seninf.c | 1330 ++++++++++++++++++++
> .../mtk-isp/isp_50/seninf/mtk_seninf_def.h | 155 +++
> .../mtk-isp/isp_50/seninf/mtk_seninf_reg.h | 965 ++++++++++++++
> 7 files changed, 2466 insertions(+)
> create mode 100644 drivers/media/platform/mtk-isp/Makefile
> create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile
> create mode 100644 drivers/media/platform/mtk-isp/isp_50/seninf/Makefile
> create mode 100644 drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf.c
> create mode 100644 drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf_def.h
> create mode 100644 drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf_reg.h
>
Thanks for the patch! Please see my comments inline.
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 7cbbd92..b0f4543 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -73,6 +73,8 @@ obj-$(CONFIG_VIDEO_ROCKCHIP_RGA) += rockchip/rga/
>
> obj-y += omap/
>
> +obj-y += mtk-isp/
> +
> obj-$(CONFIG_VIDEO_AM437X_VPFE) += am437x/
>
> obj-$(CONFIG_VIDEO_XILINX) += xilinx/
> diff --git a/drivers/media/platform/mtk-isp/Makefile b/drivers/media/platform/mtk-isp/Makefile
> new file mode 100644
> index 0000000..c17fb3f
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-y += isp_50/
> diff --git a/drivers/media/platform/mtk-isp/isp_50/Makefile b/drivers/media/platform/mtk-isp/isp_50/Makefile
> new file mode 100644
> index 0000000..8b4a792
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ifeq ($(CONFIG_MTK_SENINF),y)
> +obj-y += seninf/
> +endif
> diff --git a/drivers/media/platform/mtk-isp/isp_50/seninf/Makefile b/drivers/media/platform/mtk-isp/isp_50/seninf/Makefile
> new file mode 100644
> index 0000000..bf193fe
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/seninf/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +MODULE = mtk_seninf
> +LIB_FILES = mtk_seninf
> +
> +obj-$(CONFIG_MTK_SENINF) += mtk_seninf.o
> diff --git a/drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf.c b/drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf.c
> new file mode 100644
> index 0000000..e791110
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf.c
> @@ -0,0 +1,1330 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_graph.h>
> +#include <linux/of_irq.h>
> +#ifdef CONFIG_COMPAT
> +#include <linux/compat.h>
> +#endif
> +#include <linux/videodev2.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-async.h>
> +#include "mtk_seninf_reg.h"
> +#include "mtk_seninf_def.h"
> +
> +#define SUBDEV_SENSOR_MAIN_NAME "sensor_main"
> +#define SUBDEV_SENSOR_SUB_NAME "sensor_sub"
> +#define MTK_CAM_SENINF_PAD_MAIN_SINK 0
> +#define MTK_CAM_SENINF_PAD_SUB_SINK 1
> +#define MIPI_SENSOR_TYPE MIPI_OPHY_NCSI2
Please either add dynamic detection of the sensor type or make the driver
support only OPHY_NCSI2 sensor type, removing the other types from it.
> +
> +static inline void mt_reg_sync_writel(unsigned int v, void *a)
> +{
> + __raw_writel((v), (void __force __iomem *)((a)));
> +}
> +
> +static inline unsigned int seninf_read_reg
> + (void *reg_base, unsigned int reg_name)
> +{
> + return (unsigned int)ioread32(reg_base + reg_name);
> +}
> +
> +static inline void seninf_write_reg
> + (void *reg_base, unsigned int reg_name,
> + unsigned int value)
> +{
> + mt_reg_sync_writel(value, reg_base + reg_name);
> +}
> +
> +static inline void write_master
> + (void *reg_base, unsigned int reg_name,
> + unsigned int value, unsigned int mask)
> +{
> + mt_reg_sync_writel((ioread32(reg_base + reg_name) & ~(mask)) |
> + value, reg_base + reg_name);
> +}
Could we just use writel() and readl() directly everywhere in the driver, as
is the common practice in the Linux code base?
> +
> +static inline int is_4d1c(unsigned int port)
> +{
> + return (port < CFG_CSI_PORT_0A);
> +}
> +
> +static inline int is_cdphy_combo(unsigned int port)
> +{
> + return (port == CFG_CSI_PORT_0A ||
> + port == CFG_CSI_PORT_0B ||
> + port == CFG_CSI_PORT_0);
> +}
> +
> +static struct seninf_csi_info SENINF_CSI_INFO[CFG_CSI_PORT_MAX_NUM] = {
Please use lowercase for variable names.
> + {CFG_CSI_PORT_0, SENINF_1},
> + {CFG_CSI_PORT_1, SENINF_3},
> + {CFG_CSI_PORT_2, SENINF_5},
> + {CFG_CSI_PORT_0A, SENINF_1},
> + {CFG_CSI_PORT_0B, SENINF_2},
> +};
This seems to be a simple mapping from a continuous range of integers into
different integers. Could we just have a const array accessed directly in a
helper function? I.e.
static u32 seninf_csi_port_to_seninf(u32 port)
{
static const u32 port_to_seninf[] = {
[CFG_CSI_PORT_0] = SENINF_1,
[CFG_CSI_PORT_1] = SENINF_3,
// ...
};
if (WARN_ON(port >= ARRAY_SIZE(port_to_seninf)))
return -1; // Or some other value we know is incorrect.
return port_to_seninf[port];
}
> +
> +struct _seninf {
It's uncommon to prefix struct names with _. I'd suggest renaming to
mtk_seninf.
> + struct v4l2_subdev subdev;
> + struct v4l2_async_notifier notifier;
> + struct device *dev;
> + struct v4l2_fwnode_endpoint ep[NUM_PORTS];
> + struct v4l2_ctrl_handler ctrl_handler;
> + struct media_pad pads[NUM_PADS];
> + struct clk *cam_clk, *top_mux_clk;
> + unsigned int port;
> + struct v4l2_subdev_format fmt;
> + unsigned int mux_select;
> + void __iomem *base_reg;
> + void __iomem *rx_reg;
> + unsigned char *csi2_rx[CFG_CSI_PORT_MAX_NUM];
> +};
> +
> +static int set_top_mux_ctrl(struct _seninf *priv,
> + unsigned int mux_idx,
> + unsigned int seninf_src)
Could you prefix the functions in this driver with mtk_seninf_ so they are
easy to find with ctags and similar tools?
> +{
> + void *pseninf = priv->base_reg;
> +
> + seninf_write_reg(pseninf, SENINF_TOP_MUX_CTRL,
> + ((seninf_read_reg(pseninf, SENINF_TOP_MUX_CTRL) &
> + (~(0xF << (mux_idx * 4)))) | ((seninf_src & 0xF)
> + << (mux_idx * 4))));
> +
> + return 0;
> +}
Do we need a whole function for just one register write?
> +
> +static void set_mux_ctrl
> + (struct _seninf *priv, unsigned int mux,
> + unsigned int input_data_type)
> +{
> + void *pseninf = priv->base_reg + 0x1000 * mux;
> + unsigned int temp;
> + unsigned int pix_sel_ext;
> + unsigned int pix_sel;
> + unsigned int hs_pol = 0;
> + unsigned int vs_pol = 0;
> + unsigned int pixel_mode = ONE_PIXEL_MODE;
> + unsigned int src_type_sel = MIPI_SENSOR;
> +
> + write_master(pseninf, SENINF1_MUX_CTRL,
> + (src_type_sel << 12), 0x0000F000);
> + temp = (src_type_sel == TEST_MODEL) ? 0 : 1;
nit: This could be simplified to (src_type_sel != TEST_MODEL).
> + write_master(pseninf, SENINF1_MUX_CTRL_EXT,
> + (temp << 0), 0x00000003);
No need to shift if the shift is 0.
> +
> + switch (pixel_mode) {
> + case 1: /* 2 Pixel */
> + pix_sel_ext = 0;
> + pix_sel = 1 << 8;
> + break;
> + case 2: /* 4 Pixel */
> + pix_sel_ext = 1 << 4;
> + pix_sel = 0;
> + break;
> + default: /* 1 Pixel */
> + pix_sel_ext = 0;
> + pix_sel = 0;
> + break;
> + }
> +
> + write_master(pseninf, SENINF1_MUX_CTRL_EXT, pix_sel_ext, 0x00000010);
> + write_master(pseninf, SENINF1_MUX_CTRL, pix_sel, 0x00000100);
> +
> + if (input_data_type != JPEG_FMT)
> + write_master(pseninf, SENINF1_MUX_CTRL,
> + (2 << 28), 0x30000000);
> + else
> + write_master(pseninf, SENINF1_MUX_CTRL, 0, 0x30000000);
This looks like something that would be normally written as:
val = 0;
if (input_data_type != JPEG_FMT)
val |= SOME_BITFIELD_VALUE_MACRO;
write_master(..., val, <mask>);
> +
> + if (src_type_sel == CSI2 || src_type_sel >= MIPI_SENSOR) {
> + /* Need to use Default for New design */
> + if (input_data_type != JPEG_FMT)
> + write_master(pseninf, SENINF1_MUX_CTRL,
> + ((0x1B << 22) | (0x1F << 16)),
> + 0x0FFF0000);
> + else
> + write_master(pseninf, SENINF1_MUX_CTRL,
> + ((0x18 << 22) | (0x1E << 16)),
> + 0x0FFF0000);
> + }
> +
> + write_master(pseninf, SENINF1_MUX_CTRL,
> + ((hs_pol << 10) | (vs_pol << 9)), 0x00000600);
> +
> + temp = seninf_read_reg(pseninf, SENINF1_MUX_CTRL);
> + seninf_write_reg(pseninf, SENINF1_MUX_CTRL, temp | 0x3);
> + seninf_write_reg(pseninf, SENINF1_MUX_CTRL, temp & 0xFFFFFFFC);
Please define macros for the magic numbers.
> +}
> +
> +static void enable_mux(struct _seninf *priv, unsigned int mux)
> +{
> + void *pseninf = priv->base_reg + 0x1000 * mux;
> +
> + write_master(pseninf, SENINF1_MUX_CTRL, (1 << 31), 0x80000000);
> +}
Do we need a separate function for this one register write?
> +
> +static struct seninf_csi_info *get_csi_info(struct _seninf *priv,
> + unsigned int mipi_port)
> +{
> + int i;
> +
> + for (i = 0; i < CFG_CSI_PORT_MAX_NUM; i++) {
> + if (SENINF_CSI_INFO[i].port == mipi_port)
> + return &SENINF_CSI_INFO[i];
> + }
> +
> + return NULL;
> +}
> +
> +static void set_dphy
> + (struct _seninf *priv,
> + struct seninf_csi_mipi *pcsi_mipi)
> +{
> + struct seninf_csi_info *csi_info = pcsi_mipi->csi_info;
> + void *pmipi_rx_base = priv->csi2_rx[CFG_CSI_PORT_0];
> + void *pmipi_rx = priv->csi2_rx[csi_info->port];
> + void *pmipi_rx_conf = priv->base_reg + 0x1000 * csi_info->seninf;
> +
> + /* Set analog phy mode to DPHY */
> + if (is_cdphy_combo(csi_info->port))
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A, 0, 0x00000001);
Don't we need to write 1 there otherwise?
> + /* 4D1C: MIPIRX_ANALOG_A_BASE = 0x00001A42 */
> + if (is_4d1c(csi_info->port))
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A,
> + (1 << 6) | (1 << 9) | (1 << 11) | (1 << 12),
> + 0x00001B60);
> + else /* MIPIRX_ANALOG_BASE = 0x102 */
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A,
> + (1 << 8), 0x00001B60);
> +
> + if (is_cdphy_combo(csi_info->port))
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B, 0, 0x00000001);
Don't we also need to write 1 there othwerise?
> +
> + /* Only 4d1c need set CSIB: MIPIRX_ANALOG_B_BASE = 0x00001242 */
> + if (is_4d1c(csi_info->port))
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B,
> + (1 << 6) | (1 << 9) | (1 << 12), 0x00001B60);
> + else /* MIPIRX_ANALOG_BASE = 0x102 */
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B,
> + (1 << 8) | (1 << 9), 0x00001B40);
Shouldn't the mask be the same in both cases?
> +
> + /* Byte clock invert */
> + write_master(pmipi_rx, MIPI_RX_ANAA8_CSI0A,
> + (1 << 0) | (1 << 1) | (1 << 2), 0x00000007);
> + if (is_4d1c(csi_info->port))
> + write_master(pmipi_rx, MIPI_RX_ANAA8_CSI0B,
> + (1 << 0) | (1 << 1) | (1 << 2),
> + 0x00000007);
Don't we need to write the opposite values in the other case?
> +
> + /* Start ANA EQ tuning */
> + if (is_cdphy_combo(csi_info->port)) {
> + write_master(pmipi_rx, MIPI_RX_ANA18_CSI0A,
> + (1 << 4) | (1 << 6), 0x000000F0);
> + write_master(pmipi_rx, MIPI_RX_ANA1C_CSI0A,
> + (1 << 20) | (1 << 22), 0x00F00000);
> + write_master(pmipi_rx, MIPI_RX_ANA20_CSI0A,
> + (1 << 20) | (1 << 22), 0x00F00000);
> +
> + if (is_4d1c(csi_info->port)) { /* 4d1c */
> + write_master(pmipi_rx, MIPI_RX_ANA18_CSI0B,
> + (1 << 4) | (1 << 6), 0x000000F0);
> + write_master(pmipi_rx, MIPI_RX_ANA1C_CSI0B,
> + (1 << 20) | (1 << 22),
> + 0x00F00000);
> + write_master(pmipi_rx, MIPI_RX_ANA20_CSI0B,
> + (1 << 20) | (1 << 22),
> + 0x00F00000);
> + }
> + } else {
> + write_master(pmipi_rx, MIPI_RX_ANA18_CSI1A,
> + (1 << 4) | (1 << 6) | (1 << 20) |
> + (1 << 22), 0x00F000F0);
> + write_master(pmipi_rx, MIPI_RX_ANA1C_CSI1A,
> + (1 << 4) | (1 << 6), 0x000000F0);
> +
> + if (is_4d1c(csi_info->port)) { /* 4d1c */
> + write_master(pmipi_rx,
> + MIPI_RX_ANA18_CSI1B, (1 << 4) |
> + (1 << 6) | (1 << 20) |
> + (1 << 22), 0x00F000F0);
> + write_master(pmipi_rx,
> + MIPI_RX_ANA1C_CSI1B, (1 << 4) |
> + (1 << 6), 0x000000F0);
> + }
> + }
> +
> + /* End ANA EQ tuning */
> + seninf_write_reg(pmipi_rx_base, MIPI_RX_ANA40_CSI0A, 0x90);
> + write_master(pmipi_rx, MIPI_RX_ANA24_CSI0A,
> + (0x40 << 24), 0xFF000000);
> + if (is_4d1c(csi_info->port))
> + write_master(pmipi_rx, MIPI_RX_ANA24_CSI0B,
> + (0x40 << 24), 0xFF000000);
> + write_master(pmipi_rx, MIPI_RX_WRAPPER80_CSI0A, 0, 0x00030000);
> + if (is_4d1c(csi_info->port))
> + write_master(pmipi_rx, MIPI_RX_WRAPPER80_CSI0B, 0, 0x00030000);
> + /* ANA power on */
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A,
> + (1 << 3), 0x00000008);
> + if (is_4d1c(csi_info->port))
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B,
> + (1 << 3), 0x00000008);
> + usleep_range(20, 40);
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A,
> + (1 << 3), 0x00000008);
> + if (is_4d1c(csi_info->port))
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B,
> + (1 << 2), 0x00000004);
> + udelay(1);
> + /* 4d1c: MIPIRX_CONFIG_CSI_BASE = 0xC9000000; */
> + if (is_4d1c(csi_info->port)) {
> + write_master(pmipi_rx_conf, MIPI_RX_CON24_CSI0,
> + (1 << 24) | (2 << 26) | (3 << 30), 0xFF000000);
> + } else { /* 2d1c: MIPIRX_CONFIG_CSI_BASE = 0xE4000000; */
> + write_master(pmipi_rx_conf, MIPI_RX_CON24_CSI0,
> + (1 << 26) | (2 << 28) | (3 << 30), 0xFF000000);
> + }
Please define macros for the magic numbers.
> + pr_debug("pcsi_mipi->CSI2_IP %d, MIPI_RX_CON24_CSI0 0x%x\n",
> + csi_info->port,
> + seninf_read_reg(pmipi_rx_conf, MIPI_RX_CON24_CSI0));
> + usleep_range(20, 40);
> + /* D-PHY SW Delay Line calibration */
What is this comment about? The delay?
> +}
> +
> +static void set_cphy
> + (struct _seninf *priv,
> + struct seninf_csi_mipi *pcsi_mipi)
> +{ /* Cphy setting for CSI0 */
Perhaps renaming the function to mtk_seninf_set_csi0_cphy() could eliminate
the need for this comment?
> + struct seninf_csi_info *csi_info = pcsi_mipi->csi_info;
> + void *pmipi_rx = priv->csi2_rx[csi_info->port];
> + int status;
> + int i;
> +
> + /* Byte clock invert */
> + write_master(pmipi_rx, MIPI_RX_ANAA8_CSI0A,
> + (1 << 0) | (1 << 2), 0x00000005);
> + write_master(pmipi_rx, MIPI_RX_ANAA8_CSI0B,
> + (1 << 0) | (1 << 2), 0x00000005);
> + /* EQ Power to Enhance Speed */
> + write_master(pmipi_rx, MIPI_RX_ANA18_CSI0A,
> + (1 << 6) | (1 << 22), 0x00C000C0);
> + write_master(pmipi_rx, MIPI_RX_ANA1C_CSI0A,
> + (1 << 6) | (1 << 22), 0x00C000C0);
> + write_master(pmipi_rx, MIPI_RX_ANA20_CSI0A,
> + (1 << 6) | (1 << 22), 0x00C000C0);
> + write_master(pmipi_rx, MIPI_RX_ANA18_CSI0B,
> + (1 << 6) | (1 << 22), 0x00C000C0);
> + write_master(pmipi_rx, MIPI_RX_ANA1C_CSI0B,
> + (1 << 6) | (1 << 22), 0x00C000C0);
> + write_master(pmipi_rx, MIPI_RX_ANA20_CSI0B,
> + (1 << 6) | (1 << 22), 0x00C000C0);
> +
> + /* CDR register setting */
> +
> + *((int *)(priv->csi2_rx[csi_info->port] + 0x30)) = 0x06040404;
> + *((int *)(priv->csi2_rx[csi_info->port] + 0x3c)) = 0x06040404;
> + *((int *)(priv->csi2_rx[csi_info->port] + 0x34)) = 0x1;
> + *((int *)(priv->csi2_rx[csi_info->port] + 0x28)) = 0x1;
> +
> + *((int *)(priv->csi2_rx[csi_info->port] + 0x1030)) =
> + 0x06040404;
> + *((int *)(priv->csi2_rx[csi_info->port] + 0x103c)) =
> + 0x06040404;
> + *((int *)(priv->csi2_rx[csi_info->port] + 0x1034)) = 0x1;
> + *((int *)(priv->csi2_rx[csi_info->port] + 0x1028)) = 0x1;
Please don't access IO memory directly. Always use some accessor,
preferrably writel().
> +
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A,
> + (1 << 0), 0x00001B61);
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B,
> + (1 << 0), 0x00001B61);
> + /* Power on DPHY */
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A,
> + (1 << 3), 0x00000008);
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B,
> + (1 << 3), 0x00000008);
> + usleep_range(20, 40);
> + /* Enable LPF */
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A,
> + (1 << 2), 0x00000004);
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B,
> + (1 << 2), 0x00000004);
> + udelay(1);
> + /* Offset calibration */
> + write_master(pmipi_rx, MIPI_RX_ANA18_CSI0A,
> + (1 << 0) | (1 << 16), 0x00010001);
> + write_master(pmipi_rx, MIPI_RX_ANA1C_CSI0A,
> + (1 << 0) | (1 << 16), 0x00010001);
> + write_master(pmipi_rx, MIPI_RX_ANA20_CSI0A,
> + (1 << 0) | (1 << 16), 0x00010001);
> + write_master(pmipi_rx, MIPI_RX_ANA18_CSI0B,
> + (1 << 0) | (1 << 16), 0x00010001);
> + write_master(pmipi_rx, MIPI_RX_ANA1C_CSI0B,
> + (1 << 0) | (1 << 16), 0x00010001);
> + write_master(pmipi_rx, MIPI_RX_ANA20_CSI0B,
> + (1 << 0) | (1 << 16), 0x00010001);
> + udelay(1);
Please define macros for the magic bits and values.
> +
> + i = 0;
> + while (1) {
> + status = seninf_read_reg(pmipi_rx,
> + MIPI_RX_ANA48_CSI0A);
> + if ((status & 0x3f) == 0x3f)
> + break;
> + i++;
> + if (i > 100) {
> + pr_debug("CSIA offset calibration timeout\n");
This is an error. Please print with error log level, e.g. dev_err(),
return an error from this function and make sure the caller correctly
handles errors by cleaning up the steps performed before.
> + break;
> + }
> + usleep_range(20, 40);
> + }
Could this be replaced with a readl_poll_timeout()?
> +
> + i = 0;
> + while (1) {
> + status = seninf_read_reg(pmipi_rx,
> + MIPI_RX_ANA48_CSI0B);
> + if ((status & 0x3f) == 0x3f)
> + break;
> + i++;
> + if (i > 100) {
> + pr_debug("CSIB offset calibration timeout\n");
> + break;
> + }
> + usleep_range(20, 40);
> + }
Ditto.
> +}
> +
> +static void set_csi_mipi
> + (struct _seninf *priv,
> + struct seninf_csi_mipi *pcsi_mipi)
> +{
> + struct seninf_csi_info *csi_info = pcsi_mipi->csi_info;
> + void *seninf_base = priv->base_reg;
> + void *pseninf = priv->base_reg + 0x1000 * csi_info->seninf;
> + void *pmipi_rx = priv->csi2_rx[csi_info->port];
> + unsigned int cal_sel;
> + unsigned int mipi_enable = 1;
> + unsigned int data_header_order = 1;
> + unsigned int pad_sel = PAD_10BIT;
> + unsigned int csi_type = (MIPI_SENSOR_TYPE == MIPI_CPHY)
> + ? CSI2_2_5G_CPHY : CSI2_2_5G;
Couldn't we just check MIPI_SENSOR_TYPE directly?
> +
> + pr_debug("IS_4D1C %d csi_type %d port %d\n",
> + is_4d1c(csi_info->port), csi_type, csi_info->port);
> +
> + switch (csi_info->port) {
> + case CFG_CSI_PORT_1:
> + cal_sel = 1;
> + write_master(seninf_base, SENINF_TOP_PHY_SENINF_CTL_CSI1,
> + ((2 << 8) | (mipi_enable << 31)), 0x80000701);
> + break;
> + case CFG_CSI_PORT_2:
> + cal_sel = 2;
> + write_master(seninf_base, SENINF_TOP_PHY_SENINF_CTL_CSI2,
> + ((2 << 8) | (mipi_enable << 31)), 0x80000701);
> + break;
> + case CFG_CSI_PORT_0:
> + cal_sel = 0;
> + write_master(seninf_base, SENINF_TOP_PHY_SENINF_CTL_CSI0,
> + ((2 << 8) |
> + (mipi_enable << 31)), 0x80000701);
> + break;
> + case CFG_CSI_PORT_0A:
> + case CFG_CSI_PORT_0B:
> + cal_sel = 0;
> + write_master(seninf_base, SENINF_TOP_PHY_SENINF_CTL_CSI0,
> + ((1 << 8) | (1 << 12) |
> + (mipi_enable << 31)), 0x80003701);
> + break;
> + default:
> + pr_err("unsupported CSI configuration\n");
> + cal_sel = 0;
> + write_master(seninf_base, SENINF_TOP_PHY_SENINF_CTL_CSI0,
> + ((2 << 8) |
> + (mipi_enable << 31)), 0x80000701);
> + break;
Could we make this case impossible to happen by the design of the driver?
> + }
> +
> + /* First Enable Sensor interface and select pad (0x1a04_0200) */
> + write_master(pseninf, SENINF1_CTRL,
> + mipi_enable << 0, 0x00000001);
> + write_master(pseninf, SENINF1_CTRL,
> + pad_sel << 28, 0x70000000);
> +
> + if (csi_type == CSI2_1_5G || csi_type == CSI2_2_5G) {
> + write_master(pseninf, SENINF1_CTRL, 0, 0x0000F000);
> + write_master(pseninf, SENINF1_CTRL_EXT,
> + (mipi_enable << 6), 0x00000060);
> + }
> + if (!mipi_enable) {
> + seninf_write_reg(pseninf, SENINF1_CSI2_CTL,
> + seninf_read_reg(pseninf, SENINF1_CSI2_CTL) &
> + 0xFFFFFFE0);
> + /* Disable mipi BG */
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A, 0, 0x0000000C);
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B, 0, 0x0000000C);
> + return;
> + }
I can see mipi_enable is always 1. Please either make the driver handle
other types of ports fully or remove them completely.
> +
> + if (csi_type != CSI2_2_5G_CPHY)
> + set_dphy(priv, pcsi_mipi);
> + else
> + set_cphy(priv, pcsi_mipi);
> +
> + /* DPCM Enable */
> + seninf_write_reg(pseninf, SENINF1_CSI2_DPCM,
> + 1 << ((pcsi_mipi->dpcm == 0x2a)
> + ? 15 : ((pcsi_mipi->dpcm & 0xF) + 7)));
Could you refactor this to avoid using the "inline if" clause and calculate
the value for the register outside of the seninf_write_reg() function call?
> +
> + pr_debug("CSI2-%d cnt:%d LaneNum:%d CSI2_EN:%d HeadOrder:%d dpcm:%d\n",
> + cal_sel, SENINF_SETTLE_DELAY,
> + (int)(pcsi_mipi->data_lane_num + 1), (int)mipi_enable,
> + (int)data_header_order, (int)pcsi_mipi->dpcm);
> +
> + /* Settle delay */
> + write_master(pseninf, SENINF1_CSI2_LNRD_TIMING,
> + (SENINF_SETTLE_DELAY << 8), 0x0000FF00);
> + /* CSI2 control */
> + if (csi_type != CSI2_2_5G_CPHY) { /* DPhy */
> + seninf_write_reg(pseninf, SENINF1_CSI2_CTL,
> + (seninf_read_reg(pseninf, SENINF1_CSI2_CTL) |
> + (data_header_order << 16) |
> + (mipi_enable << 4) |
> + (((1 << (pcsi_mipi->data_lane_num + 1)) - 1)
> + )));
> + write_master(pseninf, SENINF1_CSI2_RESYNC_MERGE_CTL,
> + (3 << 0), 0x00000C07);
> + write_master(pseninf, SENINF1_CSI2_MODE, 0, 0x000007FF);
> + write_master(pseninf, SENINF1_CSI2_DPHY_SYNC,
> + (0xff00 << 0) | (0x001d << 16), 0xFFFFFFFF);
> + seninf_write_reg(pseninf, SENINF1_CSI2_SPARE0,
> + seninf_read_reg(pseninf, SENINF1_CSI2_SPARE0)
> + & 0xFFFFFFFE);
> + } else {
> + /* CPhy */
> + unsigned int temp;
> +
> + write_master(pseninf, SENINF1_CSI2_LNRD_TIMING, 0, 0x000000FF);
> + seninf_write_reg(pseninf, SENINF1_CSI2_CTL,
> + (seninf_read_reg(pseninf, SENINF1_CSI2_CTL) |
> + data_header_order << 16));
> + temp = (pcsi_mipi->data_lane_num == SENSOR_MIPI_1_LANE) ? 1 :
> + (pcsi_mipi->data_lane_num == SENSOR_MIPI_2_LANE) ? 2 :
> + (pcsi_mipi->data_lane_num == SENSOR_MIPI_3_LANE) ?
> + 4 : 5;
> + write_master(pseninf, SENINF1_CSI2_MODE,
> + (temp << 8), 0x00000700);
> + temp = pcsi_mipi->data_lane_num >= SENSOR_MIPI_1_LANE;
> + write_master(pseninf, SENINF1_CSI2_CTRL_TRIO_CON,
> + (temp << 0), 0x00000001);
> + temp = pcsi_mipi->data_lane_num >= SENSOR_MIPI_2_LANE;
> + write_master(pseninf, SENINF1_CSI2_CTRL_TRIO_CON,
> + (temp << 2), 0x00000004);
> + temp = pcsi_mipi->data_lane_num >= SENSOR_MIPI_3_LANE;
> + write_master(pseninf, SENINF1_CSI2_CTRL_TRIO_CON,
> + (temp << 4), 0x00000010);
> + temp = pcsi_mipi->data_lane_num >= SENSOR_MIPI_4_LANE;
> + write_master(pseninf, SENINF1_CSI2_CTRL_TRIO_CON,
> + (temp << 6), 0x00000040);
> + write_master(pseninf, SENINF1_CSI2_MODE,
> + (0x2 << 0), 0x000000FF);
> + write_master(pseninf, SENINF1_CSI2_RESYNC_MERGE_CTL,
> + (3 << 0) | (1 << 11), 0x00000C07);
> + write_master(pseninf, SENINF1_SYNC_RESYNC_CTL,
> + (1 << 0), 0x00000007);
> + write_master(pseninf, SENINF1_POST_DETECT_CTL,
> + (1 << 1), 0x00000002);
> +
> + seninf_write_reg(pseninf, SENINF1_CSI2_SPARE0,
> + seninf_read_reg(pseninf, SENINF1_CSI2_SPARE0)
> + | 0x1);
> + }
> +
> + write_master(pseninf, SENINF1_CSI2_CTL, (1 << 25), 0x0A000080);
> + write_master(pseninf, SENINF1_CSI2_HS_TRAIL,
> + (SENINF_HS_TRAIL_PARAMETER << 0), 0x000000FF);
> +
> + /* Set debug port to output packet number */
> + seninf_write_reg(pseninf, SENINF1_CSI2_DGB_SEL, 0x8000001A);
> + /* Enable CSI2 IRQ mask */
> + /* Turn on all interrupt */
> + seninf_write_reg(pseninf, SENINF1_CSI2_INT_EN, 0xFFFFFFFF);
> + /* Write clear CSI2 IRQ */
> + seninf_write_reg(pseninf, SENINF1_CSI2_INT_STATUS, 0xFFFFFFFF);
> + /* Enable CSI2 Extend IRQ mask */
> + /* Turn on all interrupt */
> + seninf_write_reg(pseninf, SENINF1_CSI2_INT_EN_EXT, 0x0000001f);
> +
> + write_master(pseninf, SENINF1_CTRL, (1 << 7), 0x00000080);
> + udelay(1);
> + write_master(pseninf, SENINF1_CTRL, 0, 0x00000080);
> +}
Please define macros for all the magic numbers.
> +
> +static void power_off(struct _seninf *priv, void *pcsi)
> +{
> + struct seninf_csi_mipi *pcsi_mipi = (struct seninf_csi_mipi *)pcsi;
> + struct seninf_csi_info *csi_info = pcsi_mipi->csi_info;
> + void *pmipi_rx = priv->csi2_rx[csi_info->port];
> + void *pseninf = priv->base_reg + 0x1000 * csi_info->seninf;
> +
> + /* Disable CSI2(2.5G) first */
> + seninf_write_reg(pseninf, SENINF1_CSI2_CTL,
> + seninf_read_reg(pseninf, SENINF1_CSI2_CTL)
> + & 0xFFFFFFE0);
> + /* Disable mipi BG */
> + switch (csi_info->port) {
> + case CFG_CSI_PORT_0A:
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A,
> + 0, 0x0000000C);
> + break;
> + case CFG_CSI_PORT_0B:
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B,
> + 0, 0x0000000C);
> + break;
> + default:
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0A,
> + 0, 0x0000000C);
> + write_master(pmipi_rx, MIPI_RX_ANA00_CSI0B,
> + 0, 0x0000000C);
Is there any reason not to just write 0 to both registers regardless of the
port? That could simplify the code.
> + break;
> + }
> +}
> +
> +static int seninf_subscribe_event(struct v4l2_subdev *sd,
> + struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub)
> +{
> + switch (sub->type) {
> + case V4L2_EVENT_CTRL:
> + return v4l2_ctrl_subdev_subscribe_event(sd, fh, sub);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int map_fmt
> + (struct _seninf *priv,
> + struct seninf_csi_mipi *pcsi_mipi)
> +{
> + int fmtidx;
> +
> + switch (priv->fmt.format.code) {
> + case MEDIA_BUS_FMT_SBGGR8_1X8:
> + case MEDIA_BUS_FMT_SGBRG8_1X8:
> + case MEDIA_BUS_FMT_SGRBG8_1X8:
> + case MEDIA_BUS_FMT_SRGGB8_1X8:
> + fmtidx = 0;
> + pcsi_mipi->dpcm = 0;
> + break;
> + case MEDIA_BUS_FMT_SGRBG10_1X10:
> + case MEDIA_BUS_FMT_SRGGB10_1X10:
> + case MEDIA_BUS_FMT_SBGGR10_1X10:
> + case MEDIA_BUS_FMT_SGBRG10_1X10:
> + fmtidx = 1;
> + pcsi_mipi->dpcm = 0;
> + break;
> + case MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8:
> + case MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8:
> + case MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8:
> + case MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8:
> + fmtidx = 0;
> + /* dpcm mode 0x2a, */
> + pcsi_mipi->dpcm = 0x2a;
> + break;
> + case MEDIA_BUS_FMT_SBGGR12_1X12:
> + case MEDIA_BUS_FMT_SGBRG12_1X12:
> + case MEDIA_BUS_FMT_SGRBG12_1X12:
> + case MEDIA_BUS_FMT_SRGGB12_1X12:
> + fmtidx = 2;
> + pcsi_mipi->dpcm = 0;
> + break;
> + case MEDIA_BUS_FMT_UYVY8_1X16:
> + case MEDIA_BUS_FMT_VYUY8_1X16:
> + case MEDIA_BUS_FMT_YUYV8_1X16:
> + case MEDIA_BUS_FMT_YVYU8_1X16:
> + fmtidx = 3;
> + pcsi_mipi->dpcm = 0;
> + break;
> + case MEDIA_BUS_FMT_JPEG_1X8:
> + case MEDIA_BUS_FMT_S5C_UYVY_JPEG_1X8:
> + fmtidx = 7;
> + pcsi_mipi->dpcm = 0;
> + break;
> + default:
> + fmtidx = 0;
> + pcsi_mipi->dpcm = 0;
> + WARN(1, "CSI2: pixel format %08x unsupported!\n",
> + priv->fmt.format.code);
> + break;
We shouldn't need to handle this as we filter out the formats in s_fmt.
> + }
> + return fmtidx;
> +}
Do you think we could split this into two functions, one that returns the
format index and one that returns the dpcm. Especially the latter could be
as simple as just handing the 4 special formats with a single if.
> +
> +static void init_fmt(struct _seninf *priv)
> +{
> + priv->fmt.format.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> + priv->fmt.format.width = DEFAULT_WIDTH;
> + priv->fmt.format.height = DEFAULT_HEIGHT;
> + priv->fmt.format.field = V4L2_FIELD_NONE;
> + priv->fmt.format.colorspace = V4L2_COLORSPACE_SRGB;
> + priv->fmt.format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> + priv->fmt.format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + priv->fmt.format.quantization = V4L2_QUANTIZATION_DEFAULT;
> +}
> +
> +static const struct v4l2_mbus_framefmt fmt_default = {
Perhaps mtk_seninf_default_fmt?
> + .code = MEDIA_BUS_FMT_SBGGR10_1X10,
> + .width = DEFAULT_WIDTH,
> + .height = DEFAULT_HEIGHT,
> + .field = V4L2_FIELD_NONE,
> + .colorspace = V4L2_COLORSPACE_SRGB,
> + .xfer_func = V4L2_XFER_FUNC_DEFAULT,
> + .ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT,
> + .quantization = V4L2_QUANTIZATION_DEFAULT,
Isn't this an exact copy of what's initialized in init_fmt()? Please replace
the whole init_fmt() with just priv->fmt.format = fmt_default.
> +};
> +
> +static int seninf_init_cfg
> + (struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg)
Something wrong seems to be going on here with line breaks.
> +{
> + struct v4l2_mbus_framefmt *mf;
> + unsigned int i;
> +
> + for (i = 0; i < sd->entity.num_pads; i++) {
> + mf = v4l2_subdev_get_try_format(sd, cfg, i);
> + *mf = fmt_default;
> + }
> +
> + return 0;
> +}
> +
> +static int seninf_set_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct _seninf *priv = container_of(sd, struct _seninf, subdev);
> +
> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> + *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format;
> + } else {
> + priv->fmt.format.code = fmt->format.code;
> + priv->fmt.format.width = fmt->format.width;
> + priv->fmt.format.height = fmt->format.height;
> + }
> +
> + pr_debug("set format 0x%x\n", priv->fmt.format.code);
> +
> + return 0;
> +}
> +
> +static int seninf_get_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct _seninf *priv = container_of(sd, struct _seninf, subdev);
> +
> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> + fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> + } else {
> + fmt->format.code = priv->fmt.format.code;
> + fmt->format.width = priv->fmt.format.width;
> + fmt->format.height = priv->fmt.format.height;
> + fmt->format.field = priv->fmt.format.field;
> + fmt->format.colorspace = priv->fmt.format.colorspace;
> + fmt->format.xfer_func = priv->fmt.format.xfer_func;
> + fmt->format.ycbcr_enc = priv->fmt.format.ycbcr_enc;
> + fmt->format.quantization = priv->fmt.format.quantization;
> + }
> + pr_debug("get format 0x%x\n", priv->fmt.format.code);
> + return 0;
> +}
Can the seninf block change the format internally or it just forwards the
data from the sensor in its original format? If the latter, we should just
propagate the format as set on the sensor here.
> +
> +static int seninf_s_stream(struct v4l2_subdev *sd, int on)
> +{
> + struct _seninf *priv = container_of(sd, struct _seninf, subdev);
> + struct seninf_csi_mipi csi_mipi;
> + struct seninf_csi_info *csi_info;
> + unsigned int mux_select;
> + unsigned int input_data_type;
> + unsigned int port;
> + unsigned int seninf_src;
> + unsigned char sensor_mipi_lane_num =
> + priv->ep[priv->port].bus.mipi_csi2.num_data_lanes;
We should just store it in some more convenient place at parsing time.
Perhaps priv->sensors[port] could be a struct that includes all the
necessary information about the sensor that isn't easily accessible by some
other means?
> + void *pseninf = priv->base_reg;
> +
> + mux_select = priv->mux_select;
nit: mux_select isn't much shorter than priv->mux_select. Could we just use
the latter directly?
> + port = priv->port;
nit: port is only used once. Could we just use priv->port directly?
> + csi_info = get_csi_info(priv, port);
> + seninf_src = csi_info->seninf;
> +
> + pr_debug("seninf csi_info->seninf(%d) port(%d)\n",
> + csi_info->seninf, csi_info->port);
dev_dbg()
In general please use the dev_() logging functions inside drivers whenever
possible.
> +
> + /* Configure Mipi */
> + csi_mipi.data_lane_num = sensor_mipi_lane_num - 1;
> + csi_mipi.csi_info = csi_info;
csi_mipi has just 3 fields. Could we pass them as arguments to the functions
that care about them?
> + input_data_type = (unsigned int)map_fmt(priv, &csi_mipi);
Why is this cast needed?
> +
> + if (on) {
> + /* Configure timestamp */
> + write_master(pseninf, SENINF1_CTRL, (1 << 0), 0x00000001);
> + write_master(pseninf, SENINF1_CTRL_EXT, (1 << 6), 0x00000040);
Please define macros for the magic numbers above.
> + seninf_write_reg(pseninf, SENINF_TG1_TM_STP,
> + SENINF_TIMESTAMP_STEP);
> +
> + set_csi_mipi(priv, &csi_mipi);
> +
> + enable_mux(priv, mux_select);
> + set_mux_ctrl(priv, mux_select, input_data_type);
> +
> + set_top_mux_ctrl(priv, mux_select, seninf_src);
> + seninf_write_reg(pseninf, SENINF_TOP_CAM_MUX_CTRL, 0x0);
> + } else {
> + power_off(priv, &csi_mipi);
> + }
> +
> + return 0;
> +};
Could move the on and !on cases to separate functions and have this one just
down to a simple
if (on)
return seninf_stream_on(...);
else
return seninf_stream_off(...);
> +
> +static const struct v4l2_subdev_pad_ops seninf_subdev_pad_ops = {
> + .init_cfg = seninf_init_cfg,
I don't think we need this callback implemented.
> + .set_fmt = seninf_set_fmt,
Can we really change the format on this device? Doesn't the format have to
match the format of the sensor? If so, we should just hardwire the format of
this device to the sensor format.
> + .get_fmt = seninf_get_fmt,
We also need .link_validate to point to v4l2_subdev_link_validate_default()
and a simple .enum_mbus_code that returns the current mbus format.
> +};
> +
> +static const struct v4l2_subdev_video_ops seninf_subdev_video_ops = {
> + .s_stream = seninf_s_stream,
> +};
> +
> +static struct v4l2_subdev_core_ops seninf_subdev_core_ops = {
> + .subscribe_event = seninf_subscribe_event,
Could we just use v4l2_ctrl_subdev_subscribe_event() here directly?
> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static struct v4l2_subdev_ops seninf_subdev_ops = {
> + .core = &seninf_subdev_core_ops,
> + .video = &seninf_subdev_video_ops,
> + .pad = &seninf_subdev_pad_ops,
> +};
> +
> +static int seninf_link_setup(struct media_entity *entity,
> + const struct media_pad *local,
> + const struct media_pad *remote, u32 flags)
> +{
> + struct v4l2_subdev *sd;
> + struct _seninf *priv;
> +
> + sd = media_entity_to_v4l2_subdev(entity);
> + priv = v4l2_get_subdevdata(sd);
> + pr_debug("mtk_seninf: remote %d-%d, local %d-%d\n",
> + remote->entity->graph_obj.id, remote->index,
> + local->entity->graph_obj.id, local->index);
> + pr_debug("local->flags %d flags %d\n", local->flags, flags);
dev_dbg()
> +
> + if ((local->flags & MEDIA_PAD_FL_SOURCE) &&
> + (flags & MEDIA_LNK_FL_ENABLED)) {
> + pr_info("set cam mux %d\n", local->index);
dev_dbg()
> +/* Select cam mux, cam mux from pads 4~11, pads 0~3 are used for sensor */
> + priv->mux_select = local->index - 4;
Can we have only 1 output of the seninf enabled?
Could we have a macro for that 4?
> + }
> +
> + if ((local->flags & MEDIA_PAD_FL_SINK) &&
> + (flags & MEDIA_LNK_FL_ENABLED)) {
> + pr_info("set port\n", local->index);
dev_dbg()
> + /* Select port */
> + priv->port = local->index;
Can we have only 1 input of the seninf enabled? I.e. is it impossible to use
2 sensors at the same time? If so, is it a hardware limitation?
> + if (priv->port > NUM_PORTS) {
Shouldn't this be >=?
That said, NUM_PORTS is defined to 2, while the comment above says the first
4 pads are used for sensors? What's the right number here?
> + pr_err("port index is over number of ports\n");
> + return -EINVAL;
> + }
> + }
> + return 0;
> +}
> +
> +static const struct media_entity_operations seninf_media_ops = {
> + .link_setup = seninf_link_setup,
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static int mtk_seninf_notifier_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *sd,
> + struct v4l2_async_subdev *asd)
> +{
> + struct _seninf *priv = container_of(notifier, struct _seninf, notifier);
> + struct device_node *node;
> + unsigned int pad, ret;
> + unsigned int is_sensor;
> + struct device_node *endpoint;
> +
> + node = to_of_node(sd->fwnode);
> + dev_info(priv->dev, "%s bounded\n", sd->entity.name);
s/bounded/bound/
Also, please make this dev_dbg().
> +
> + is_sensor = 0;
> + if (!strcmp(node->name, SUBDEV_SENSOR_MAIN_NAME)) {
Node names should not be used for any matching. Please define the port
indexes for main and sub ports (e.g. 0 and 1). See the other drivers as a
reference, for example:
https://elixir.bootlin.com/linux/v5.2/source/drivers/media/pci/intel/ipu3/ipu3-cio2.c#L1395
> + pad = MTK_CAM_SENINF_PAD_MAIN_SINK;
Related to the above, we could just use the port index as the pad index if
we define the input pads as the first N pads.
> + is_sensor = 1;
> + }
> +
> + if (!strcmp(node->name, SUBDEV_SENSOR_SUB_NAME)) {
> + pad = MTK_CAM_SENINF_PAD_SUB_SINK;
> + is_sensor = 1;
> + }
If we specify the indexes, we can just check for port < NUM_SENSOR_PORTS.
> +
> + if (is_sensor == 1) {
> + for_each_endpoint_of_node(node, endpoint) {
> + ret = v4l2_fwnode_endpoint_parse
> + (of_fwnode_handle(endpoint),
> + &priv->ep[pad]);
> + of_node_put(endpoint);
> + dev_info(priv->dev, "ep %d, bus.mipi_csi2.num_data_lanes %d\n",
> + pad,
> + priv->ep[pad].bus.mipi_csi2.num_data_lanes);
> + }
We shouldn't be parsing anything manually here. It's already parsed for us
when mtk_seninf_fwnode_parse() is called. Please implement it correctly
there.
> +
> + ret = media_create_pad_link(&sd->entity, 0,
> + &priv->subdev.entity, pad, 0);
> + dev_info
> + (priv->dev, "%s bounded with pad:%d\n",
> + sd->entity.name, pad);
> + if (ret) {
> + dev_err(priv->dev,
> + "failed to create link for %s\n",
> + sd->entity.name);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void mtk_seninf_notifier_unbind(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *sd,
> + struct v4l2_async_subdev *asd)
> +{
> + struct _seninf *priv = container_of(notifier, struct _seninf, notifier);
> +
> + dev_info(priv->dev, "%s is unbounded\n", sd->entity.name);
No need to implement if there is nothing to do.
> +}
> +
> +static const struct v4l2_async_notifier_operations mtk_seninf_async_ops = {
> + .bound = mtk_seninf_notifier_bound,
> + .unbind = mtk_seninf_notifier_unbind,
Hmm, normally one would call v4l2_device_register_subdev_nodes() from
.complete(). This driver doesn't call it at all. How does it work?
> +};
> +
> +static int mtk_seninf_fwnode_parse(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd)
> +{
This should be implemented. Please check the other drivers for reference.
For example:
https://elixir.bootlin.com/linux/v5.2/source/drivers/media/pci/intel/ipu3/ipu3-cio2.c#L1478
> + return 0;
> +}
> +
> +static int seninf_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct _seninf *priv = container_of(sd, struct _seninf, subdev);
> + int ret;
> +
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
We need to call pm_runtime_put() if get_sync() fails.
> + dev_err(priv->dev, "Failed to pm_runtime_get_sync: %d\n", ret);
> + return ret;
> + }
> +
> + clk_prepare_enable(priv->cam_clk);
> + clk_prepare_enable(priv->top_mux_clk);
clk_prepare_enable() can fail, so we want to handle errors. I'd suggest
using the clk_bulk_ API to simplify this.
We shouldn't do power management on open/release, because the userspace can
just open devices to query the drivers. We don't want any power operations
in that case. Could you move this to stream start and stop.
> +
> + return ret;
> +}
> +
> +static int seninf_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct _seninf *priv = container_of(sd, struct _seninf, subdev);
> +
> + clk_disable_unprepare(priv->top_mux_clk);
> + clk_disable_unprepare(priv->cam_clk);
> + pm_runtime_put(priv->dev);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> +static const struct v4l2_subdev_internal_ops seninf_internal_ops = {
> + .open = seninf_open,
> + .close = seninf_close,
> +};
> +#endif
> +
> +static irqreturn_t seninf_irq(int irq, void *device_id)
> +{
> + return IRQ_HANDLED;
> +}
Do we need an IRQ handler at all if there is nothing to do inside?
> +
> +static int seninf_enable_test_pattern(struct _seninf *priv, u32 pattern)
> +{
> + void *pseninf = priv->base_reg;
> +
> + switch (pattern) {
> + case TEST_DISABLED:
> + break;
Don't we need to write the registers with values for no test pattern?
> + case TEST_GEN_PATTERN:
> + seninf_write_reg(pseninf, SENINF_TOP_CTRL, 0x00000C00);
> + seninf_write_reg(pseninf, SENINF1_CTRL, 0x00001001);
> + seninf_write_reg(pseninf, SENINF1_MUX_CTRL, 0x96DF1080);
> + seninf_write_reg(pseninf, SENINF1_MUX_INTEN, 0x8000007F);
Something wrong with indentation here?
> + seninf_write_reg(pseninf, SENINF1_MUX_SPARE, 0x00000000);
> + seninf_write_reg(pseninf, SENINF1_MUX_CTRL_EXT, 0x000E2000);
> + seninf_write_reg(pseninf, SENINF1_MUX_CTRL_EXT, 0x00000000);
> + seninf_write_reg(pseninf, SENINF_TG1_TM_CTL, 0x000404C1);
> + seninf_write_reg
> + (pseninf, SENINF_TG1_TM_SIZE,
> + (priv->fmt.format.height + 0x100) << 16
> + | priv->fmt.format.width + 0x100);
The proper way to break lines of function calls is to keep the opening
parenthesis
> + seninf_write_reg(pseninf, SENINF_TG1_TM_CLK, 0x00000000);
> + seninf_write_reg(pseninf, SENINF_TG1_TM_STP, 0x1);
> + seninf_write_reg
> + (pseninf, SENINF1_CTRL_EXT, seninf_read_reg(pseninf,
> + SENINF1_CTRL_EXT) | 0x02);/* TESTMDL EN*/
Please define macros for all the magic numbers above.
> + break;
> + case TEST_DUMP_DEBUG_INFO:
> + /* Sensor Interface Control */
> + pr_info("SENINF_CSI2_CTL SENINF1:0x%x, 2:0x%x, 3:0x%x, 5:0x%x\n",
> + seninf_read_reg(pseninf, SENINF1_CSI2_CTL),
> + seninf_read_reg(pseninf, SENINF2_CSI2_CTL),
> + seninf_read_reg(pseninf, SENINF3_CSI2_CTL),
> + seninf_read_reg(pseninf, SENINF5_CSI2_CTL));
> + /* Read width/height */
> + /* Read interrupt status */
> + pr_info("SENINF1_IRQ:0x%x, 2:0x%x, 3:0x%x, 5:0x%x\n",
> + seninf_read_reg(pseninf, SENINF1_CSI2_INT_STATUS),
> + seninf_read_reg(pseninf, SENINF2_CSI2_INT_STATUS),
> + seninf_read_reg(pseninf, SENINF3_CSI2_INT_STATUS),
> + seninf_read_reg(pseninf, SENINF5_CSI2_INT_STATUS));
> + /* Mux1 */
> + pr_info("SENINF1_MUX_CTRL:0x%x, INTSTA:0x%x, DEBUG_2(0x%x)\n",
> + seninf_read_reg(pseninf, SENINF1_MUX_CTRL),
> + seninf_read_reg(pseninf, SENINF1_MUX_INTSTA),
> + seninf_read_reg(pseninf, SENINF1_MUX_DEBUG_2));
dev_dbg()
> + if (seninf_read_reg(pseninf, SENINF1_MUX_INTSTA) & 0x1) {
> + seninf_write_reg(pseninf, SENINF1_MUX_INTSTA,
> + 0xffffffff);
> + udelay(SENINF_DRV_DEBUG_DELAY);
Is this delay necessary? If so, could usleep_range() be used instead?
> + pr_info("overrun CTRL:%x INTSTA:%x DEBUG_2:%x\n",
> + seninf_read_reg(pseninf, SENINF1_MUX_CTRL),
> + seninf_read_reg(pseninf, SENINF1_MUX_INTSTA),
> + seninf_read_reg(pseninf, SENINF1_MUX_DEBUG_2));
If this is a failure, we should print with dev_err() or dev_warn() depending
on how severe the failure is. If it's a normal situation, we should use
dev_dbg().
> + }
> + break;
> + default:
> + dev_warn(priv->dev, "%s Unhandled testcase:%d\n",
> + __func__, pattern);
> + break;
This is also handled by the control framework.
> + }
> +
> + return 0;
> +}
> +
> +static int seninf_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct _seninf *priv = container_of(ctrl->handler,
> + struct _seninf, ctrl_handler);
> + int ret;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_TEST_PATTERN:
> + ret = seninf_enable_test_pattern(priv, ctrl->val);
Could we just return from here directly?
> + break;
> + default:
> + ret = 0;
> + dev_warn(priv->dev, "%s Unhandled id:0x%x, val:0x%x\n",
> + __func__, ctrl->id, ctrl->val);
> + break;
There is no need to handle this. The control core will handle invalid
requests for us.
> + }
> +
> + return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops seninf_ctrl_ops = {
> + .s_ctrl = seninf_set_ctrl,
> +};
> +
> +static const char * const seninf_test_pattern_menu[] = {
> + "Disabled",
> + "generate_test_pattern",
> + "dump_debug_info",
Could we have some more human-readable names describing the pattern itself?
Like "Horizontal bars" or "Color gradient".
> +};
> +
> +static int seninf_initialize_controls(struct _seninf *priv)
> +{
> + struct v4l2_ctrl_handler *handler;
> + struct v4l2_ctrl *test_pattern;
> + int ret;
> +
> + handler = &priv->ctrl_handler;
> + ret = v4l2_ctrl_handler_init(handler, 2);
Why 2?
> + if (ret)
> + return ret;
> + test_pattern =
Why do you need this assignment? The variable is not used anywhere later.
> + v4l2_ctrl_new_std_menu_items(handler, &seninf_ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(seninf_test_pattern_menu) - 1,
> + 0, 0, seninf_test_pattern_menu);
> +
> + if (handler->error) {
> + ret = handler->error;
> + dev_err(priv->dev,
> + "Failed to init controls(%d)\n", ret);
> + goto err_free_handler;
> + }
> +
> + priv->subdev.ctrl_handler = handler;
> + return 0;
> +
> +err_free_handler:
> + v4l2_ctrl_handler_free(handler);
> +
> + return ret;
> +}
> +
> +static int seninf_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + struct _seninf *priv;
> + struct v4l2_subdev *sd;
> + struct device *dev = &pdev->dev;
> + struct media_pad *pads;
> + unsigned int irq_info[3];
> + unsigned int irq;
> + int i;
> + int ret;
> +
> + dev_dbg(dev, "seninf probe +\n");
> + priv = devm_kzalloc(&pdev->dev, sizeof(struct _seninf), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
nit: Add a blank line here to improve readability. Generally please add
blank lines between unrelated blocks of code.
> + memset(priv, 0, sizeof(struct _seninf));
Use sizeof(*priv) instead to make this work regardless of the type.
> + priv->dev = &pdev->dev;
> + sd = &priv->subdev;
> + pads = priv->pads;
nit: Add a blank line here.
> + /* Get IRQ ID and request IRQ */
> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
Use platform_get_irq() instead, as the platform_device subsystem kindly
parsed the IRQs for us already.
> +
nit: This blank line is not needed as the lines above and below are related.
> + if (!irq) {
> + pr_debug("No IRQ found!!\n");
> + return 0;
0 means success, but I think we failed here, right?
> + }
> + /* Get IRQ Flag from device node */
> + if (of_property_read_u32_array(pdev->dev.of_node,
> + "interrupts", irq_info,
> + ARRAY_SIZE(irq_info))) {
> + dev_err(dev, "get irq flags from DTS fail!!\n");
> + return -ENODEV;
> + }
> + ret = request_irq(irq, seninf_irq,
> + irq_info[2], "SENINF", NULL);
No need to parse and pass the flags here, the platform device subsystem
already handled them. 0 should be fine.
> + if (ret) {
> + dev_err(dev, "request_irq fail\n");
> + return ret;
> + }
> + pr_debug("Seninf devnode:%s, irq=%d\n",
> + pdev->dev.of_node->name, irq);
IMHO this debug log is a bit excessive, please remove.
Also, in general, please use dev_dbg().
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "base_reg");
Please avoid relying on the names, especially since we have only 2 register
sets here and both are mandatory. Please just define the DT bindings with
explicit order and use indexes.
> + priv->base_reg = devm_ioremap_resource(dev, res);
> + if (IS_ERR(priv->base_reg))
> + return PTR_ERR(priv->base_reg);
nit: Add blank line.
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rx_reg");
> + priv->rx_reg = devm_ioremap_resource(dev, res);
> + if (IS_ERR(priv->rx_reg))
> + return PTR_ERR(priv->rx_reg);
> +
> + priv->csi2_rx[CFG_CSI_PORT_0] = priv->rx_reg;
> + priv->csi2_rx[CFG_CSI_PORT_0A] = priv->rx_reg;
Hmm, is it expected that Port 0 and 0A use the same registers?
> + priv->csi2_rx[CFG_CSI_PORT_0B] = priv->rx_reg + 0x1000;
> + priv->csi2_rx[CFG_CSI_PORT_1] = priv->rx_reg + 0x2000;
> + priv->csi2_rx[CFG_CSI_PORT_2] = priv->rx_reg + 0x4000;
> +
> + priv->cam_clk = devm_clk_get(dev, "CLK_CAM_SENINF");
Please use lowercase and "-" instead of "_" in clock names. There is also no
need to prefix clock names with "clk" and suffix with "seninf". The right
names here would be "cam" and "top-mux".
> + if (IS_ERR(priv->cam_clk)) {
> + dev_err(dev, "Failed to get cam_clk\n");
> + return -EINVAL;
> + }
> +
> + priv->top_mux_clk = devm_clk_get(dev, "CLK_TOP_MUX_SENINF");
> + if (IS_ERR(priv->top_mux_clk)) {
> + dev_err(dev, "Failed to get top_mux_clk\n");
> + return -EINVAL;
> + }
I'd suggest moving out the Media and V4L2 registration code into a separate
function. Starting from this line.
> +
> + v4l2_subdev_init(sd, &seninf_subdev_ops);
> +
> + init_fmt(priv);
> + ret = seninf_initialize_controls(priv);
> + if (ret) {
> + dev_err(dev, "Failed to initialize controls\n");
> + return ret;
> + }
> +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
The driver should be made depend on this in Kconfig and ifdefs removed.
> + sd->internal_ops = &seninf_internal_ops;
> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
Merge the two lines into one.
> +#endif
> + priv->subdev.dev = &pdev->dev;
Why not just sd->dev?
> + snprintf(sd->name, V4L2_SUBDEV_NAME_SIZE, "%s.mipi-csi",
> + dev_name(&pdev->dev));
The ".mipi-csi" suffix is not needed.
> + v4l2_set_subdevdata(sd, priv);
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> + sd->entity.ops = &seninf_media_ops;
nit: Blank line here.
> + for (i = 0; i < 4; i++)
Perhaps define a macro for this 4?
> + pads[i].flags = MEDIA_PAD_FL_SINK;
> + for (i = 4; i < NUM_PADS; i++)
> + pads[i].flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&sd->entity, NUM_PADS, pads);
> + if (ret < 0)
> + goto err_free_handler;
> +#endif
> + ret = v4l2_async_notifier_parse_fwnode_endpoints
> + (dev, &priv->notifier, sizeof(struct v4l2_async_subdev),
> + mtk_seninf_fwnode_parse);
> + if (ret < 0)
> + goto err_clean_entity;
> +
> + if (!priv->notifier.num_subdevs) {
> + ret = -ENODEV; /* no endpoint */
> + goto err_clean_entity;
> + }
> +
> + priv->subdev.subdev_notifier = &priv->notifier;
> + priv->notifier.ops = &mtk_seninf_async_ops;
> + ret = v4l2_async_subdev_notifier_register(sd, &priv->notifier);
> + if (ret < 0) {
> + dev_err(dev, "v4l2 async notifier register failed\n");
> + goto err_clean_notififer;
> + }
> +
> + ret = v4l2_async_register_subdev(sd);
> + if (ret < 0) {
> + dev_err(dev, "v4l2 async register subdev failed\n");
> + goto err_clean_notififer;
> + }
The code from the previous comment and until this line deserves a separate function.
> + pm_runtime_set_active(dev);
> + pm_runtime_enable(dev);
> + pm_runtime_idle(dev);
We should only need pm_runtime_enable() here.
> + dev_info(dev, "seninf probe -\n");
> + return 0;
> +
> +err_clean_notififer:
> + v4l2_async_notifier_cleanup(&priv->notifier);
> +err_clean_entity:
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + media_entity_cleanup(&sd->entity);
> +#endif
> +err_free_handler:
> + v4l2_ctrl_handler_free(&priv->ctrl_handler);
> +
> + return ret;
> +}
> +
> +static int seninf_pm_suspend(struct device *dev)
> +{
> + pr_debug("seninf_runtime_suspend\n");
> +
> + return 0;
> +}
> +
> +static int seninf_pm_resume(struct device *dev)
> +{
> + pr_debug("seninf_runtime_resume\n");
> +
> + return 0;
> +}
There should be no need to provide empty callbacks if there isn't anything
to do here.
> +
> +static const struct dev_pm_ops runtime_pm_ops = {
> + SET_RUNTIME_PM_OPS(seninf_pm_suspend,
> + seninf_pm_resume,
> + NULL)
No need to break the line here.
> +};
> +
> +static int seninf_remove(struct platform_device *pdev)
> +{
> + struct v4l2_subdev *subdev = platform_get_drvdata(pdev);
> + struct _seninf *priv = container_of(subdev, struct _seninf, subdev);
One would normally set the driver-specific struct as drvdata.
> + struct v4l2_subdev *sd = &priv->subdev;
Isn't this the same as subdev?
> +
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + media_entity_cleanup(&sd->entity);
> +#endif
This driver doesn't make sense without Media Controller. Let's just make it
depend on it in Kconfig and remove the ifdefs.
> + v4l2_ctrl_handler_free(&priv->ctrl_handler);
There are still devices exposed that are using this control handler at this
point. This should happen after unregistering those devices.
> + v4l2_async_unregister_subdev(&priv->subdev);
> + pm_runtime_disable(priv->dev);
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id mtk_seninf_of_match[] = {
> + {.compatible = "mediatek,mt8183-seninf"},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, mtk_seninf_of_match);
> +#endif
> +
> +static int seninf_suspend(struct platform_device *pdev, pm_message_t mesg)
> +{
> + return 0;
> +}
> +
> +static int seninf_resume(struct platform_device *pdev)
> +{
> + return 0;
> +}
These 2 should be implemented. Specifically, any hardware state needs to be
preserved across system suspend, so that any streaming that started before
suspend automatically keeps streaming after resuming.
> +
> +static struct platform_driver seninf_pdrv = {
> + .probe = seninf_probe,
> + .remove = seninf_remove,
> + .suspend = seninf_suspend,
> + .resume = seninf_resume,
These suspend/resume fields are deprecated. Please set these in dev_pm_ops,
not here.
> + .driver = {
> + .name = "seninf",
Maybe mtk-seninf?
> + .owner = THIS_MODULE,
> +#ifdef CONFIG_OF
> + .of_match_table = mtk_seninf_of_match,
> +#endif
No need for this ifdef if you use the of_match_ptr() macro.
> + .pm = &runtime_pm_ops,
> + },
> +};
> +
> +static int __init mtk_seninf_init(void)
> +{
> + int ret;
> +
> + pr_debug("seninf Init\n");
> + ret = platform_driver_register(&seninf_pdrv);
> + if (ret) {
> + pr_err("Failed to register platform driver\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void __exit mtk_seninf_exit(void)
> +{
> + pr_debug("seninf Exit\n");
> + platform_driver_unregister(&seninf_pdrv);
> +}
> +
> +late_initcall(mtk_seninf_init);
Why late_initcall()? You should be able to just use module_platform_driver()
here. If not, that would be a bug somewhere in this or another related
driver.
> +module_exit(mtk_seninf_exit);
> +
> +MODULE_DESCRIPTION("MTK seninf driver");
> +MODULE_AUTHOR("Louis Kuo <louis.kuo@mediatek.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("MTK:mtk_seninf");
> diff --git a/drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf_def.h b/drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf_def.h
> new file mode 100644
> index 0000000..8d00b88
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf_def.h
> @@ -0,0 +1,155 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __SENINF_DRV_DEF_H__
> +#define __SENINF_DRV_DEF_H__
> +
> +#if defined(SENINF_CLK_208M)
> +#define SENINF_TIMESTAMP_STEP 0x67
> +#define SENINF_SETTLE_DELAY 0x15
> +#define SENINF_HS_TRAIL_PARAMETER 0x8
> +#elif defined(SENINF_CLK_312M)
> +#define SENINF_TIMESTAMP_STEP 0x9b
> +#define SENINF_SETTLE_DELAY 0x20
> +#define SENINF_HS_TRAIL_PARAMETER 0xa
> +#else
> +#define SENINF_TIMESTAMP_STEP 0x67
> +#define SENINF_SETTLE_DELAY 0x15
> +#define SENINF_HS_TRAIL_PARAMETER 0x8
> +#endif
Please detect the clock frequency at runtime and use the right value
dynamically.
> +
> +#define SENINF_DRV_DEBUG_DELAY 1000
> +
> +#define SENSOR_CLOCK_POLARITY_HIGH 0
> +#define SENSOR_CLOCK_POLARITY_LOW 1
These two don't seem to be used.
> +#define NUM_PADS 12
> +#define NUM_PORTS 2
> +#define DEFAULT_WIDTH 1600
> +#define DEFAULT_HEIGHT 1200
> +
> +enum {
> + MIPI_OPHY_NCSI2 = 0,
> + MIPI_OPHY_CSI2 = 1,
> + MIPI_CPHY = 2,
> +};
> +
> +enum {
> + TEST_DISABLED = 0X0,
> + TEST_GEN_PATTERN,
> + TEST_DUMP_DEBUG_INFO,
> +};
> +
> +enum {
> + CFG_CSI_PORT_0 = 0x0, /* 4D1C */
> + CFG_CSI_PORT_1, /* 4D1C */
> + CFG_CSI_PORT_2, /* 4D1C */
> + CFG_CSI_PORT_0A, /* 2D1C */
> + CFG_CSI_PORT_0B, /* 2D1C */
> + CFG_CSI_PORT_MAX_NUM,
> + CFG_CSI_PORT_NONE /*for non-MIPI sensor */
> +};
> +
> +enum {
> + ONE_PIXEL_MODE = 0x0,
> + TWO_PIXEL_MODE = 0x1,
> + FOUR_PIXEL_MODE = 0x2,
> +};
> +
> +#define SENINF_CAM_MUX_MIN SENINF_MUX1
> +#define SENINF_CAM_MUX_MAX SENINF_MUX3
> +#define SENINF_CAMSV_MUX_MIN SENINF_MUX3
> +#define SENINF_CAMSV_MUX_MAX SENINF_MUX_NUM
> +
> +#define SENINF_PIXEL_MODE_CAM TWO_PIXEL_MODE
> +#define SENINF_PIXEL_MODE_CAMSV FOUR_PIXEL_MODE
> +
> +#define SENINF_TIMESTAMP_CLK 1000
> +
The 7 macros above are not used.
> +enum {
> + SENSOR_MIPI_1_LANE = 0,
> + SENSOR_MIPI_2_LANE,
> + SENSOR_MIPI_3_LANE,
> + SENSOR_MIPI_4_LANE
> +};
Why not just use a normal int here? It's just a value between 1 and 4.
> +
> +enum {
> + SENINF_MUX1 = 0x0,
> + SENINF_MUX2 = 0x1,
> + SENINF_MUX3 = 0x2,
> + SENINF_MUX4 = 0x3,
> + SENINF_MUX5 = 0x4,
> + SENINF_MUX6 = 0x5,
> + SENINF_MUX_NUM,
> + SENINF_MUX_ERROR = -1,
> +};
This enum is not used.
> +
> +enum {
> + SENINF_1 = 0x0,
> + SENINF_2 = 0x1,
> + SENINF_3 = 0x2,
> + SENINF_4 = 0x3,
> + SENINF_5 = 0x4,
> + SENINF_NUM,
> +};
> +
> +enum {
> + PAD_10BIT = 0x0,
> + PAD_8BIT_7_0 = 0x3,
> + PAD_8BIT_9_2 = 0x4,
> +};
Is this enum used?
> +
> +enum { /* 0:CSI2(2.5G), 3: parallel, 8:NCSI2(1.5G) */
> + CSI2 = 0x0, /* 2.5G support */
> + TEST_MODEL = 0x1,
> + CCIR656 = 0x2,
> + PARALLEL_SENSOR = 0x3,
> + SERIAL_SENSOR = 0x4,
> + HD_TV = 0x5,
> + EXT_CSI2_OUT1 = 0x6,
> + EXT_CSI2_OUT2 = 0x7,
> + MIPI_SENSOR = 0x8,/* 1.5G support */
> + VIRTUAL_CHANNEL_1 = 0x9,
> + VIRTUAL_CHANNEL_2 = 0xA,
> + VIRTUAL_CHANNEL_3 = 0xB,
> + VIRTUAL_CHANNEL_4 = 0xC,
> + VIRTUAL_CHANNEL_5 = 0xD,
> + VIRTUAL_CHANNEL_6 = 0xE,
> +};
Is this enum used?
> +
> +enum { /* 0:CSI2(2.5G), 1:NCSI2(1.5G) */
> + CSI2_1_5G = 0x0, /* 1.5G support */
> + CSI2_2_5G = 0x1, /* 2.5G support*/
> + CSI2_2_5G_CPHY = 0x2, /* 2.5G support*/
> +};
> +
> +enum {
> + RAW_8BIT_FMT = 0x0,
> + RAW_10BIT_FMT = 0x1,
> + RAW_12BIT_FMT = 0x2,
> + YUV422_FMT = 0x3,
> + RAW_14BIT_FMT = 0x4,
> + RGB565_MIPI_FMT = 0x5,
> + RGB888_MIPI_FMT = 0x6,
> + JPEG_FMT = 0x7
> +};
> +
> +enum {
> + CMD_SENINF_GET_SENINF_ADDR,
> + CMD_SENINF_DEBUG_TASK,
> + CMD_SENINF_DEBUG_TASK_CAMSV,
> + CMD_SENINF_DEBUG_PIXEL_METER,
> + CMD_SENINF_MAX
> +};
Is this enum used?
> +
> +/* MIPI sensor pad usage */
> +struct seninf_csi_info {
> + unsigned int port;
> + unsigned int seninf;
> +};
> +
> +struct seninf_csi_mipi {
> + struct seninf_csi_info *csi_info;
> + unsigned int data_lane_num;
> + unsigned int dpcm;
> +};
Please use a space between type name and variable name.
> +
> +#endif /*__SENINF_DRV_DEF_H__ */
> diff --git a/drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf_reg.h b/drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf_reg.h
> new file mode 100644
> index 0000000..349dc9c
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/seninf/mtk_seninf_reg.h
> @@ -0,0 +1,965 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _SENINF_REG_H_
> +#define _SENINF_REG_H_
> +
> +/* 0x11C80000..0x11C850AC */
> +#define MIPI_RX_ANA00_CSI0A 0x0000
> +#define MIPI_RX_ANA04_CSI0A 0x0004
> +#define MIPI_RX_ANA08_CSI0A 0x0008
> +#define MIPI_RX_ANA0C_CSI0A 0x000C
Please use lowercase for hex numbers.
> +#define MIPI_RX_ANA10_CSI0A 0x0010
> +#define MIPI_RX_ANA14_CSI0A 0x0014
> +#define MIPI_RX_ANA18_CSI0A 0x0018
> +#define MIPI_RX_ANA1C_CSI0A 0x001C
> +#define MIPI_RX_ANA20_CSI0A 0x0020
> +#define MIPI_RX_ANA24_CSI0A 0x0024
> +#define MIPI_RX_ANA28_CSI0A 0x0028
> +#define MIPI_RX_ANA2C_CSI0A 0x002C
> +#define rsv_0030 0x0030
Please remove reserved addresses.
> +#define MIPI_RX_ANA34_CSI0A 0x0034
> +#define MIPI_RX_ANA38_CSI0A 0x0038
> +#define rsv_003C 0x003C
> +#define MIPI_RX_ANA40_CSI0A 0x0040
> +#define rsv_0044 0x0044
> +#define MIPI_RX_ANA48_CSI0A 0x0048
> +#define rsv_004C_13 0x004C
> +#define MIPI_RX_WRAPPER80_CSI0A 0x0080
> +#define MIPI_RX_WRAPPER84_CSI0A 0x0084
> +#define MIPI_RX_WRAPPER88_CSI0A 0x0088
> +#define MIPI_RX_WRAPPER8C_CSI0A 0x008C
> +#define MIPI_RX_WRAPPER90_CSI0A 0x0090
> +#define MIPI_RX_WRAPPER94_CSI0A 0x0094
> +#define MIPI_RX_WRAPPER98_CSI0A 0x0098
> +#define MIPI_RX_WRAPPER9C_CSI0A 0x009C
> +#define rsv_00A0 0x00A0
> +#define MIPI_RX_ANAA4_CSI0A 0x00A4
> +#define MIPI_RX_ANAA8_CSI0A 0x00A8
> +#define rsv_00AC_981 0x00AC
> +#define MIPI_RX_ANA00_CSI0B 0x1000
> +#define MIPI_RX_ANA04_CSI0B 0x1004
> +#define MIPI_RX_ANA08_CSI0B 0x1008
> +#define MIPI_RX_ANA0C_CSI0B 0x100C
> +#define MIPI_RX_ANA10_CSI0B 0x1010
> +#define MIPI_RX_ANA14_CSI0B 0x1014
> +#define MIPI_RX_ANA18_CSI0B 0x1018
> +#define MIPI_RX_ANA1C_CSI0B 0x101C
> +#define MIPI_RX_ANA20_CSI0B 0x1020
> +#define MIPI_RX_ANA24_CSI0B 0x1024
> +#define MIPI_RX_ANA28_CSI0B 0x1028
> +#define MIPI_RX_ANA2C_CSI0B 0x102C
> +#define rsv_1030 0x1030
> +#define MIPI_RX_ANA34_CSI0B 0x1034
> +#define MIPI_RX_ANA38_CSI0B 0x1038
> +#define rsv_103C_3 0x103C
> +#define MIPI_RX_ANA48_CSI0B 0x1048
> +#define rsv_104C_13 0x104C
> +#define MIPI_RX_WRAPPER80_CSI0B 0x1080
> +#define MIPI_RX_WRAPPER84_CSI0B 0x1084
> +#define MIPI_RX_WRAPPER88_CSI0B 0x1088
> +#define MIPI_RX_WRAPPER8C_CSI0B 0x108C
> +#define MIPI_RX_WRAPPER90_CSI0B 0x1090
> +#define MIPI_RX_WRAPPER94_CSI0B 0x1094
> +#define MIPI_RX_WRAPPER98_CSI0B 0x1098
> +#define MIPI_RX_WRAPPER9C_CSI0B 0x109C
> +#define rsv_10A0 0x10A0
> +#define MIPI_RX_ANAA4_CSI0B 0x10A4
> +#define MIPI_RX_ANAA8_CSI0B 0x10A8
> +#define rsv_10AC_981 0x10AC
> +#define MIPI_RX_ANA00_CSI1A 0x2000
> +#define MIPI_RX_ANA04_CSI1A 0x2004
> +#define MIPI_RX_ANA08_CSI1A 0x2008
> +#define MIPI_RX_ANA0C_CSI1A 0x200C
> +#define MIPI_RX_ANA10_CSI1A 0x2010
> +#define rsv_2014 0x2014
> +#define MIPI_RX_ANA18_CSI1A 0x2018
> +#define MIPI_RX_ANA1C_CSI1A 0x201C
> +#define rsv_2020 0x2020
> +#define MIPI_RX_ANA24_CSI1A 0x2024
> +#define rsv_2028_8 0x2028
> +#define MIPI_RX_ANA48_CSI1A 0x2048
> +#define rsv_204C_13 0x204C
> +#define MIPI_RX_WRAPPER80_CSI1A 0x2080
> +#define MIPI_RX_WRAPPER84_CSI1A 0x2084
> +#define MIPI_RX_WRAPPER88_CSI1A 0x2088
> +#define MIPI_RX_WRAPPER8C_CSI1A 0x208C
> +#define MIPI_RX_WRAPPER90_CSI1A 0x2090
> +#define MIPI_RX_WRAPPER94_CSI1A 0x2094
> +#define MIPI_RX_WRAPPER98_CSI1A 0x2098
> +#define MIPI_RX_WRAPPER9C_CSI1A 0x209C
> +#define rsv_20A0 0x20A0
> +#define MIPI_RX_ANAA4_CSI1A 0x20A4
> +#define MIPI_RX_ANAA8_CSI1A 0x20A8
> +#define rsv_20AC_981 0x20AC
> +#define MIPI_RX_ANA00_CSI1B 0x3000
> +#define MIPI_RX_ANA04_CSI1B 0x3004
> +#define MIPI_RX_ANA08_CSI1B 0x3008
> +#define MIPI_RX_ANA0C_CSI1B 0x300C
> +#define MIPI_RX_ANA10_CSI1B 0x3010
> +#define rsv_3014 0x3014
> +#define MIPI_RX_ANA18_CSI1B 0x3018
> +#define MIPI_RX_ANA1C_CSI1B 0x301C
> +#define rsv_3020 0x3020
> +#define MIPI_RX_ANA24_CSI1B 0x3024
> +#define rsv_3028_8 0x3028
> +#define MIPI_RX_ANA48_CSI1B 0x3048
> +#define rsv_304C_13 0x304C
> +#define MIPI_RX_WRAPPER80_CSI1B 0x3080
> +#define MIPI_RX_WRAPPER84_CSI1B 0x3084
> +#define MIPI_RX_WRAPPER88_CSI1B 0x3088
> +#define MIPI_RX_WRAPPER8C_CSI1B 0x308C
> +#define MIPI_RX_WRAPPER90_CSI1B 0x3090
> +#define MIPI_RX_WRAPPER94_CSI1B 0x3094
> +#define MIPI_RX_WRAPPER98_CSI1B 0x3098
> +#define MIPI_RX_WRAPPER9C_CSI1B 0x309C
> +#define rsv_30A0 0x30A0
> +#define MIPI_RX_ANAA4_CSI1B 0x30A4
> +#define MIPI_RX_ANAA8_CSI1B 0x30A8
> +#define rsv_30AC_981 0x30AC
> +#define MIPI_RX_ANA00_CSI2A 0x4000
> +#define MIPI_RX_ANA04_CSI2A 0x4004
> +#define MIPI_RX_ANA08_CSI2A 0x4008
> +#define MIPI_RX_ANA0C_CSI2A 0x400C
> +#define MIPI_RX_ANA10_CSI2A 0x4010
> +#define rsv_4014 0x4014
> +#define MIPI_RX_ANA18_CSI2A 0x4018
> +#define MIPI_RX_ANA1C_CSI2A 0x401C
> +#define rsv_4020 0x4020
> +#define MIPI_RX_ANA24_CSI2A 0x4024
> +#define rsv_4028_8 0x4028
> +#define MIPI_RX_ANA48_CSI2A 0x4048
> +#define rsv_404C_13 0x404C
> +#define MIPI_RX_WRAPPER80_CSI2A 0x4080
> +#define MIPI_RX_WRAPPER84_CSI2A 0x4084
> +#define MIPI_RX_WRAPPER88_CSI2A 0x4088
> +#define MIPI_RX_WRAPPER8C_CSI2A 0x408C
> +#define MIPI_RX_WRAPPER90_CSI2A 0x4090
> +#define MIPI_RX_WRAPPER94_CSI2A 0x4094
> +#define MIPI_RX_WRAPPER98_CSI2A 0x4098
> +#define MIPI_RX_WRAPPER9C_CSI2A 0x409C
> +#define rsv_40A0 0x40A0
> +#define MIPI_RX_ANAA4_CSI2A 0x40A4
> +#define MIPI_RX_ANAA8_CSI2A 0x40A8
> +#define rsv_40AC_981 0x40AC
> +#define MIPI_RX_ANA00_CSI2B 0x5000
> +#define MIPI_RX_ANA04_CSI2B 0x5004
> +#define MIPI_RX_ANA08_CSI2B 0x5008
> +#define MIPI_RX_ANA0C_CSI2B 0x500C
> +#define MIPI_RX_ANA10_CSI2B 0x5010
> +#define rsv_5014 0x5014
> +#define MIPI_RX_ANA18_CSI2B 0x5018
> +#define MIPI_RX_ANA1C_CSI2B 0x501C
> +#define rsv_5020 0x5020
> +#define MIPI_RX_ANA24_CSI2B 0x5024
> +#define rsv_5028_8 0x5028
> +#define MIPI_RX_ANA48_CSI2B 0x5048
> +#define rsv_504C_13 0x504C
> +#define MIPI_RX_WRAPPER80_CSI2B 0x5080
> +#define MIPI_RX_WRAPPER84_CSI2B 0x5084
> +#define MIPI_RX_WRAPPER88_CSI2B 0x5088
> +#define MIPI_RX_WRAPPER8C_CSI2B 0x508C
> +#define MIPI_RX_WRAPPER90_CSI2B 0x5090
> +#define MIPI_RX_WRAPPER94_CSI2B 0x5094
> +#define MIPI_RX_WRAPPER98_CSI2B 0x5098
> +#define MIPI_RX_WRAPPER9C_CSI2B 0x509C
> +#define rsv_50A0 0x50A0
> +#define MIPI_RX_ANAA4_CSI2B 0x50A4
> +#define MIPI_RX_ANAA8_CSI2B 0x50A8
> +#define rsv_50AC_20 0x50AC
> +
> +/* 0x1A040000..0x1A047D40 */
> +#define SENINF_TOP_CTRL 0x0000
> +#define SENINF_TOP_CMODEL_PAR 0x0004
Please align all the values to the same indentation.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v4, 25/33] drm/mediatek: add clock property check before get it
From: CK Hu @ 2019-07-17 7:01 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, David Airlie, linux-kernel, dri-devel,
Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-26-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:34 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> This patch add clock property check before get it
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index c57e7ab..a9d3e27 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -668,10 +668,12 @@ static int mtk_ddp_probe(struct platform_device *pdev)
> for (i = 0; i < 10; i++)
> ddp->mutex[i].id = i;
>
> - ddp->clk = devm_clk_get(dev, NULL);
> - if (IS_ERR(ddp->clk)) {
> - dev_err(dev, "Failed to get clock\n");
> - return PTR_ERR(ddp->clk);
> + if (of_find_property(dev->of_node, "clocks", &i)) {
> + ddp->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(ddp->clk)) {
> + dev_err(dev, "Failed to get clock\n");
> + return PTR_ERR(ddp->clk);
> + }
Only "mediatek,mt8133-disp-mutex" has no clock property. For other SoC,
clock property is required. So I think this exception is just for
mt8183.
Regards,
CK
> }
>
> regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v4, 24/33] drm/mediatek: distinguish ovl and ovl_2l by layer_nr
From: CK Hu @ 2019-07-17 6:55 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, David Airlie, linux-kernel, dri-devel,
Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-25-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:34 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> distinguish ovl and ovl_2l by layer_nr when get comp
> id
>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 8ca4965..7e99827 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -326,7 +326,12 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
> if (irq < 0)
> return irq;
>
> - comp_id = mtk_ddp_comp_get_id(dev->of_node, MTK_DISP_OVL);
> + priv->data = of_device_get_match_data(dev);
> +
> + comp_id = mtk_ddp_comp_get_id(dev->of_node,
> + priv->data->layer_nr == 4 ?
> + MTK_DISP_OVL :
> + MTK_DISP_OVL_2L);
> if (comp_id < 0) {
> dev_err(dev, "Failed to identify by alias: %d\n", comp_id);
> return comp_id;
> @@ -339,8 +344,6 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
> return ret;
> }
>
> - priv->data = of_device_get_match_data(dev);
> -
> platform_set_drvdata(pdev, priv);
>
> ret = devm_request_irq(dev, irq, mtk_disp_ovl_irq_handler,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH V2 3/3] arm64: defconfig: Select CONFIG_PINCTRL_IMX8MN by default
From: Shawn Guo @ 2019-07-17 6:54 UTC (permalink / raw)
To: Anson.Huang
Cc: mark.rutland, maxime.ripard, catalin.marinas, linus.walleij,
will.deacon, stefan, bjorn.andersson, leonard.crestez, festevam,
olof, jagan, Linux-imx, devicetree, s.hauer, linux-gpio, robh+dt,
horms+renesas, linux-arm-kernel, aisheng.dong, linux-kernel,
dinguyen, kernel, enric.balletbo
In-Reply-To: <20190611122535.23583-3-Anson.Huang@nxp.com>
On Tue, Jun 11, 2019 at 08:25:35PM +0800, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
>
> Enable CONFIG_PINCTRL_IMX8MN by default to support i.MX8MN
> pinctrl driver.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v5 14/15] arm64: defconfig: Enable SDMA on i.mx8mq/8mm
From: Shawn Guo @ 2019-07-17 6:48 UTC (permalink / raw)
To: yibin.gong
Cc: robh+dt, mark.rutland, s.hauer, kernel, broonie, festevam, vkoul,
dan.j.williams, u.kleine-koenig, catalin.marinas, l.stach,
will.deacon, linux-spi, linux-imx, linux-arm-kernel, linux-kernel,
dmaengine, devicetree
In-Reply-To: <20190610081753.11422-15-yibin.gong@nxp.com>
On Mon, Jun 10, 2019 at 04:17:52PM +0800, yibin.gong@nxp.com wrote:
> From: Robin Gong <yibin.gong@nxp.com>
>
> Enable SDMA support on i.mx8mq/8mm chips, including enabling
> CONFIG_FW_LOADER_USER_HELPER/CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> for firmware loaded by udev.
>
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v4, 23/33] drm/mediatek: add ovl0/ovl_2l0 usecase
From: CK Hu @ 2019-07-17 6:47 UTC (permalink / raw)
To: yongqiang.niu
Cc: Mark Rutland, devicetree, Philipp Zabel, David Airlie,
linux-kernel, dri-devel, Rob Herring, linux-mediatek,
Daniel Vetter, Matthias Brugger, linux-arm-kernel
In-Reply-To: <1562625253-29254-24-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:34 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> This patch add ovl0/ovl_2l0 usecase
> in ovl->ovl_2l0 direct link usecase:
> 1. the crtc support layer number will 4+2
> 2. ovl_2l0 background color input select ovl0 when crtc init
> and disable it when crtc finish
> 3. config ovl_2l0 layer, if crtc config layer number is
> bigger than ovl0 support layers(max is 4)
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 38 +++++++++++++++++++++++++++++++--
> 1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 5eac376..9ee9ce2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -282,6 +282,15 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>
> for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i];
> + enum mtk_ddp_comp_id prev;
> +
> + if (i > 0)
> + prev = mtk_crtc->ddp_comp[i - 1]->id;
> + else
> + prev = DDP_COMPONENT_ID_MAX;
> +
> + if (prev == DDP_COMPONENT_OVL0)
> + mtk_ddp_comp_bgclr_in_on(comp);
I does not like to use a specific component id to check, that is not
general. For now, you could simply call mtk_ddp_comp_bgclr_in_on(comp);
for all component because only ovl_2l has implemented it.
Regards,
CK
>
> mtk_ddp_comp_config(comp, width, height, vrefresh, bpc);
> mtk_ddp_comp_start(comp);
> @@ -291,9 +300,18 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> for (i = 0; i < mtk_crtc->layer_nr; i++) {
> struct drm_plane *plane = &mtk_crtc->planes[i];
> struct mtk_plane_state *plane_state;
> + struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
> + unsigned int comp_layer_nr = mtk_ddp_comp_layer_nr(comp);
> + unsigned int local_layer;
>
> plane_state = to_mtk_plane_state(plane->state);
> - mtk_ddp_comp_layer_config(mtk_crtc->ddp_comp[0], i,
> +
> + if (i >= comp_layer_nr) {
> + comp = mtk_crtc->ddp_comp[1];
> + local_layer = i - comp_layer_nr;
> + } else
> + local_layer = i;
> + mtk_ddp_comp_layer_config(comp , local_layer,
> plane_state);
> }
>
> @@ -319,6 +337,7 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc)
> mtk_crtc->ddp_comp[i]->id);
> mtk_disp_mutex_disable(mtk_crtc->mutex);
> for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) {
> + mtk_ddp_comp_bgclr_in_off(mtk_crtc->ddp_comp[i]);
> mtk_ddp_remove_comp_from_path(mtk_crtc->config_regs,
> mtk_crtc->mmsys_reg_data,
> mtk_crtc->ddp_comp[i]->id,
> @@ -339,6 +358,8 @@ static void mtk_crtc_ddp_config(struct drm_crtc *crtc)
> struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);
> struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
> unsigned int i;
> + unsigned int comp_layer_nr = mtk_ddp_comp_layer_nr(comp);
> + unsigned int local_layer;
>
> /*
> * TODO: instead of updating the registers here, we should prepare
> @@ -361,7 +382,14 @@ static void mtk_crtc_ddp_config(struct drm_crtc *crtc)
> plane_state = to_mtk_plane_state(plane->state);
>
> if (plane_state->pending.config) {
> - mtk_ddp_comp_layer_config(comp, i, plane_state);
> + if (i >= comp_layer_nr) {
> + comp = mtk_crtc->ddp_comp[1];
> + local_layer = i - comp_layer_nr;
> + } else
> + local_layer = i;
> +
> + mtk_ddp_comp_layer_config(comp, local_layer,
> + plane_state);
> plane_state->pending.config = false;
> }
> }
> @@ -592,6 +620,12 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
> }
>
> mtk_crtc->layer_nr = mtk_ddp_comp_layer_nr(mtk_crtc->ddp_comp[0]);
> + if (mtk_crtc->ddp_comp_nr > 1) {
> + struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[1];
> +
> + if (comp->funcs->bgclr_in_on)
> + mtk_crtc->layer_nr += mtk_ddp_comp_layer_nr(comp);
> + }
> mtk_crtc->planes = devm_kcalloc(dev, mtk_crtc->layer_nr,
> sizeof(struct drm_plane),
> GFP_KERNEL);
^ permalink raw reply
* Re: [PATCH v5 13/15] ARM: dts: imx6sll: correct sdma compatible
From: Shawn Guo @ 2019-07-17 6:42 UTC (permalink / raw)
To: yibin.gong
Cc: robh+dt, mark.rutland, s.hauer, kernel, broonie, festevam, vkoul,
dan.j.williams, u.kleine-koenig, catalin.marinas, l.stach,
will.deacon, linux-spi, linux-imx, linux-arm-kernel, linux-kernel,
dmaengine, devicetree
In-Reply-To: <20190610081753.11422-14-yibin.gong@nxp.com>
On Mon, Jun 10, 2019 at 04:17:51PM +0800, yibin.gong@nxp.com wrote:
> From: Robin Gong <yibin.gong@nxp.com>
>
> Correct sdma compatible since ecspi errata ERR009165 has been fixed
> on i.mx6sll as i.mx6ul.
>
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v5 12/15] ARM: dts: imx6ul: add dma support on ecspi
From: Shawn Guo @ 2019-07-17 6:42 UTC (permalink / raw)
To: yibin.gong
Cc: mark.rutland, devicetree, linux-kernel, catalin.marinas, s.hauer,
will.deacon, linux-imx, linux-spi, vkoul, robh+dt, broonie,
kernel, u.kleine-koenig, dmaengine, dan.j.williams, festevam,
linux-arm-kernel, l.stach
In-Reply-To: <20190610081753.11422-13-yibin.gong@nxp.com>
On Mon, Jun 10, 2019 at 04:17:50PM +0800, yibin.gong@nxp.com wrote:
> From: Robin Gong <yibin.gong@nxp.com>
>
> Add dma support on ecspi.
>
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-17 6:36 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <20190717093317.70fefb27@dimatab>
On 7/16/19 11:33 PM, Dmitry Osipenko wrote:
> В Tue, 16 Jul 2019 22:55:52 -0700
> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>
>> On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
>>> В Tue, 16 Jul 2019 22:25:25 -0700
>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>
>>>> On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
>>>>> В Tue, 16 Jul 2019 19:35:49 -0700
>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>
>>>>>> On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:
>>>>>>> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
>>>>>>>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
>>>>>>>>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>>>>>>>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>>>>>>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>>>>>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph
>>>>>>>>>>>>>>>>>>>>> Lo wrote:
>>>>>>>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>>>>>>>>>> The other thing that also need attention is
>>>>>>>>>>>>>>>>>>>>>>>> that T124 CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed
>>>>>>>>>>>>>>>>>>>>>>>> first, which is
>>>>>>>>>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Should I add check for successful dfll clk
>>>>>>>>>>>>>>>>>>>>>>> register explicitly in
>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk
>>>>>>>>>>>>>>>>>>>>>>> registers?
>>>>>>>>>>>>>>>>>>>> Probably you should use the "device links". See
>>>>>>>>>>>>>>>>>>>> [1][2] for the
>>>>>>>>>>>>>>>>>>>> example.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if
>>>>>>>>>>>>>>>>>>>> device_link_add() fails.
>>>>>>>>>>>>>>>>>>>> And
>>>>>>>>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's
>>>>>>>>>>>>>>>>>>>> device, see [3].
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> [3]
>>>>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Will go thru and add...
>>>>>>>>>>>>>>>>> Looks like I initially confused this case with getting
>>>>>>>>>>>>>>>>> orphaned clock.
>>>>>>>>>>>>>>>>> I'm now seeing that the DFLL driver registers the
>>>>>>>>>>>>>>>>> clock and then
>>>>>>>>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until
>>>>>>>>>>>>>>>>> DFLL driver is
>>>>>>>>>>>>>>>>> probed, hence everything should be fine as-is and
>>>>>>>>>>>>>>>>> there is no real
>>>>>>>>>>>>>>>>> need
>>>>>>>>>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just
>>>>>>>>>>>>>>>>>>>>>> regarding the DFLL
>>>>>>>>>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU
>>>>>>>>>>>>>>>>>>>>>> clock sources and
>>>>>>>>>>>>>>>>>>>>>> integrated with DVFS control logic with the
>>>>>>>>>>>>>>>>>>>>>> regulator. We will not
>>>>>>>>>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>>>>>>>>>> CPU to other clock sources once we switched to
>>>>>>>>>>>>>>>>>>>>>> DFLL. Because the
>>>>>>>>>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table
>>>>>>>>>>>>>>>>>>>>>> (CVB or OPP
>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to
>>>>>>>>>>>>>>>>>>>>>> other sources with
>>>>>>>>>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We
>>>>>>>>>>>>>>>>>>>>>> allow switching to
>>>>>>>>>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce
>>>>>>>>>>>>>>>>>>>> DFLL freq to
>>>>>>>>>>>>>>>>>>>> PLLP's
>>>>>>>>>>>>>>>>>>>> rate before switching to PLLP in order to have a
>>>>>>>>>>>>>>>>>>>> proper CPU voltage.
>>>>>>>>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no
>>>>>>>>>>>>>>>>>>> need to enforce
>>>>>>>>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source
>>>>>>>>>>>>>>>>>>> to PLLP during
>>>>>>>>>>>>>>>>>>> suspend
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Sorry, please ignore my above comment. During
>>>>>>>>>>>>>>>>>> suspend, need to change
>>>>>>>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop
>>>>>>>>>>>>>>>>>> mode first and
>>>>>>>>>>>>>>>>>> then
>>>>>>>>>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>>>>>>>>>> Okay.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> And I don't exactly understand why we need to
>>>>>>>>>>>>>>>>>>>>>> switch to PLLP in
>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the
>>>>>>>>>>>>>>>>>>>>>> time.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it
>>>>>>>>>>>>>>>>>>>>>> the open-loop
>>>>>>>>>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest
>>>>>>>>>>>>>>>>>>>>>> of the sequence to
>>>>>>>>>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the
>>>>>>>>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And leave
>>>>>>>>>>>>>>>>>>>>>> it on PLL_P.
>>>>>>>>>>>>>>>>>>>>>> After
>>>>>>>>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore
>>>>>>>>>>>>>>>>>>>>>> the CPU clock
>>>>>>>>>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to
>>>>>>>>>>>>>>>>>>>>>> close-loop mode.
>>>>>>>>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL
>>>>>>>>>>>>>>>>>>>> parent during of
>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence
>>>>>>>>>>>>>>>>>>>> instead of having
>>>>>>>>>>>>>>>>>>>> odd
>>>>>>>>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a
>>>>>>>>>>>>>>>>>>>> proper suspend-resume sequencing of the device
>>>>>>>>>>>>>>>>>>>> drivers. In this case
>>>>>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>>>>>> driver is the driver that enables DFLL and switches
>>>>>>>>>>>>>>>>>>>> CPU to that
>>>>>>>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>>>>>>>> source, which means that this driver is also should
>>>>>>>>>>>>>>>>>>>> be responsible for
>>>>>>>>>>>>>>>>>>>> management of the DFLL's state during of
>>>>>>>>>>>>>>>>>>>> suspend/resume process. If
>>>>>>>>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and
>>>>>>>>>>>>>>>>>>>> re-enables it
>>>>>>>>>>>>>>>>>>>> during
>>>>>>>>>>>>>>>>>>>> resume, then looks like the CaR driver hacks around
>>>>>>>>>>>>>>>>>>>> DFLL are not
>>>>>>>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the
>>>>>>>>>>>>>>>>>>>>>> patch subject to
>>>>>>>>>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>>>>>>>>>> suspend-resume support" seems more appropriate to
>>>>>>>>>>>>>>>>>>>>>> me.
>>>>>>>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use are as
>>>>>>>>>>>>>>>>>>>>> follows (assuming
>>>>>>>>>>>>>>>>>>>>> all
>>>>>>>>>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU
>>>>>>>>>>>>>>>>>>>>> frequency is ok for
>>>>>>>>>>>>>>>>>>>>> any
>>>>>>>>>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I see during switch away from DFLL (suspend), cclk_g
>>>>>>>>>>>>>>>>>> parent is not
>>>>>>>>>>>>>>>>>> changed to PLLP before changing dfll to open loop
>>>>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Will add this ...
>>>>>>>>>>>>>>>>> The CPUFreq driver switches parent to PLLP during the
>>>>>>>>>>>>>>>>> probe, similar
>>>>>>>>>>>>>>>>> should be done on suspend.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I'm also wondering if it's always safe to switch to
>>>>>>>>>>>>>>>>> PLLP in the probe.
>>>>>>>>>>>>>>>>> If CPU is running on a lower freq than PLLP, then some
>>>>>>>>>>>>>>>>> other more
>>>>>>>>>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always
>>>>>>>>>>>>>>>> runs at higher
>>>>>>>>>>>>>>>> rate
>>>>>>>>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to
>>>>>>>>>>>>>>>> dfll clock enable
>>>>>>>>>>>>>>>> should be safe.
>>>>>>>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a
>>>>>>>>>>>>>>> divided output of
>>>>>>>>>>>>>>> PLLP
>>>>>>>>>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Probably, realistically, CPU is always running off a
>>>>>>>>>>>>>>> fast PLLX during
>>>>>>>>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I
>>>>>>>>>>>>>>> guess ideally CPUFreq driver should also have a
>>>>>>>>>>>>>>> 'shutdown' callback to teardown DFLL
>>>>>>>>>>>>>>> on a reboot, but likely that there are other
>>>>>>>>>>>>>>> clock-related problems as
>>>>>>>>>>>>>>> well that may break KEXEC and thus it is not very
>>>>>>>>>>>>>>> important at the
>>>>>>>>>>>>>>> moment.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> [snip]
>>>>>>>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source
>>>>>>>>>>>>>> above I meant
>>>>>>>>>>>>>> PLL_P_OUT4.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> As per clock policies, PLL_X is always used for high freq
>>>>>>>>>>>>>> like
>>>>>>>>>>>>>>> 800Mhz
>>>>>>>>>>>>>> and for low frequency it will be sourced from PLLP.
>>>>>>>>>>>>> Alright, then please don't forget to pre-initialize
>>>>>>>>>>>>> PLLP_OUT4 rate to a
>>>>>>>>>>>>> reasonable value using tegra_clk_init_table or
>>>>>>>>>>>>> assigned-clocks.
>>>>>>>>>>>> PLLP_OUT4 rate update is not needed as it is safe to run at
>>>>>>>>>>>> 408Mhz because it is below fmax @ Vmin
>>>>>>>>>>> So even 204MHz CVB entries are having the same voltage as
>>>>>>>>>>> 408MHz, correct? It's not instantly obvious to me from the
>>>>>>>>>>> DFLL driver's code where the fmax @ Vmin is defined, I see
>>>>>>>>>>> that there is the min_millivolts
>>>>>>>>>>> and frequency entries starting from 204MHZ defined
>>>>>>>>>>> per-table.
>>>>>>>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will
>>>>>>>>>> work at Vmin voltage and PLLP max is 408Mhz.
>>>>>>>>> Thank you for the clarification. It would be good to have that
>>>>>>>>> commented
>>>>>>>>> in the code as well.
>>>>>>>> OK, Will add...
>>>>>>> Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend
>>>>>>> happens very early even before disabling non-boot CPUs and also
>>>>>>> need to export clock driver APIs to CPUFreq.
>>>>>>>
>>>>>>> Was thinking of below way of implementing this...
>>>>>>>
>>>>>>>
>>>>>>> Clock DFLL driver Suspend:
>>>>>>>
>>>>>>> - Save CPU clock policy registers, and Perform dfll
>>>>>>> suspend which sets in open loop mode
>>>>>>>
>>>>>>> CPU Freq driver Suspend: does nothing
>>>>>>>
>>>>>>>
>>>>>>> Clock DFLL driver Resume:
>>>>>>>
>>>>>>> - Re-init DFLL, Set in Open-Loop mode, restore CPU
>>>>>>> Clock policy registers which actually sets source to DFLL along
>>>>>>> with other CPU Policy register restore.
>>>>>>>
>>>>>>> CPU Freq driver Resume:
>>>>>>>
>>>>>>> - do clk_prepare_enable which acutally sets DFLL in
>>>>>>> Closed loop mode
>>>>>>>
>>>>>>>
>>>>>>> Adding one more note: Switching CPU Clock to PLLP is not needed
>>>>>>> as CPU CLock can be from dfll in open-loop mode as DFLL is not
>>>>>>> disabled anywhere throught the suspend/resume path and SC7 entry
>>>>>>> FW and Warm boot code will switch CPU source to PLLP.
>>>>> Since CPU resumes on PLLP, it will be cleaner to suspend it on
>>>>> PLLP as well. And besides, seems that currently disabling DFLL
>>>>> clock will disable DFLL completely and then you'd want to re-init
>>>>> the DFLL on resume any ways. So better to just disable DFLL
>>>>> completely on suspend, which should happen on clk_disable(dfll).
>>>> Will switch to PLLP during CPUFreq suspend. With decision of using
>>>> clk_disable during suspend, its mandatory to switch to PLLP as DFLL
>>>> is completely disabled.
>>>>
>>>> My earlier concern was on restoring CPU policy as we can't do that
>>>> from CPUFreq driver and need export from clock driver.
>>>>
>>>> Clear now and will do CPU clock policy restore in after dfll
>>>> re-init.
>>> Why the policy can't be saved/restored by the CaR driver as a
>>> context of any other clock?
>> restoring cpu clock policy involves programming source and
>> super_cclkg_divider.
>>
>> cclk_g is registered as clk_super_mux and it doesn't use frac_div ops
>> to do save/restore its divider.
> That can be changed of course and I guess it also could be as simple as
> saving and restoring of two raw u32 values of the policy/divider
> registers.
>
>> Also, during clock context we cant restore cclk_g as cclk_g source
>> will be dfll and dfll will not be resumed/re-initialized by the time
>> clk_super_mux save/restore happens.
>>
>> we can't use save/restore context for dfll clk_ops because
>> dfllCPU_out parent to CCLK_G is first in the clock tree and dfll_ref
>> and dfll_soc peripheral clocks are not restored by the time dfll
>> restore happens. Also dfll peripheral clock enables need to be
>> restored before dfll restore happens which involves programming dfll
>> controller for re-initialization.
>>
>> So dfll resume/re-init is done in clk-tegra210 at end of all clocks
>> restore in V5 series but instead of in clk-tegra210 driver I moved
>> now to dfll-fcpu driver pm_ops as all dfll dependencies will be
>> restored thru clk_restore_context by then. This will be in V6.
> Since DFLL is now guaranteed to be disabled across CaR suspend/resume
> (hence it has nothing to do in regards to CCLK) and given that PLLs
> state is restored before the rest of the clocks, I don't see why not to
> implement CCLK save/restore in a generic fasion. CPU policy wull be
> restored to either PLLP or PLLX (if CPUFreq driver is disabled).
>
CCLK_G save/restore should happen in clk_super_mux ops save/context and
clk_super_mux save/restore happens very early as cclk_g is first in the
clock tree and save/restore traverses through the tree top-bottom order.
DFLL enable thru CPUFreq resume happens after all clk_restore_context
happens. So during clk_restore_context, dfll re-init doesnt happen and
doing cpu clock policy restore during super_mux clk_ops will crash as
DFLL is not initialized and its clock is not enabled but CPU clock
restore sets source to DFLL if we restore during super_clk_mux
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 6:33 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <093462f3-8c6d-d084-9822-ae4eff041c64@nvidia.com>
В Tue, 16 Jul 2019 22:55:52 -0700
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
> On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
> > В Tue, 16 Jul 2019 22:25:25 -0700
> > Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
> >
> >> On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
> >>> В Tue, 16 Jul 2019 19:35:49 -0700
> >>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
> >>>
> >>>> On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:
> >>>>> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
> >>>>>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
> >>>>>>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
> >>>>>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
> >>>>>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
> >>>>>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
> >>>>>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
> >>>>>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
> >>>>>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
> >>>>>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
> >>>>>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
> >>>>>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
> >>>>>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
> >>>>>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
> >>>>>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph
> >>>>>>>>>>>>>>>>>>> Lo wrote:
> >>>>>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
> >>>>>>>>>>>>>>>>>>>>>> The other thing that also need attention is
> >>>>>>>>>>>>>>>>>>>>>> that T124 CPUFreq
> >>>>>>>>>>>>>>>>>>>>>> driver
> >>>>>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed
> >>>>>>>>>>>>>>>>>>>>>> first, which is
> >>>>>>>>>>>>>>>>>>>>>> icky.
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> Should I add check for successful dfll clk
> >>>>>>>>>>>>>>>>>>>>> register explicitly in
> >>>>>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk
> >>>>>>>>>>>>>>>>>>>>> registers?
> >>>>>>>>>>>>>>>>>> Probably you should use the "device links". See
> >>>>>>>>>>>>>>>>>> [1][2] for the
> >>>>>>>>>>>>>>>>>> example.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> [1]
> >>>>>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> [2]
> >>>>>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if
> >>>>>>>>>>>>>>>>>> device_link_add() fails.
> >>>>>>>>>>>>>>>>>> And
> >>>>>>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's
> >>>>>>>>>>>>>>>>>> device, see [3].
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> [3]
> >>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> Will go thru and add...
> >>>>>>>>>>>>>>> Looks like I initially confused this case with getting
> >>>>>>>>>>>>>>> orphaned clock.
> >>>>>>>>>>>>>>> I'm now seeing that the DFLL driver registers the
> >>>>>>>>>>>>>>> clock and then
> >>>>>>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until
> >>>>>>>>>>>>>>> DFLL driver is
> >>>>>>>>>>>>>>> probed, hence everything should be fine as-is and
> >>>>>>>>>>>>>>> there is no real
> >>>>>>>>>>>>>>> need
> >>>>>>>>>>>>>>> for the 'device link'. Sorry for the confusion!
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just
> >>>>>>>>>>>>>>>>>>>> regarding the DFLL
> >>>>>>>>>>>>>>>>>>>> part.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU
> >>>>>>>>>>>>>>>>>>>> clock sources and
> >>>>>>>>>>>>>>>>>>>> integrated with DVFS control logic with the
> >>>>>>>>>>>>>>>>>>>> regulator. We will not
> >>>>>>>>>>>>>>>>>>>> switch
> >>>>>>>>>>>>>>>>>>>> CPU to other clock sources once we switched to
> >>>>>>>>>>>>>>>>>>>> DFLL. Because the
> >>>>>>>>>>>>>>>>>>>> CPU has
> >>>>>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table
> >>>>>>>>>>>>>>>>>>>> (CVB or OPP
> >>>>>>>>>>>>>>>>>>>> table
> >>>>>>>>>>>>>>>>>>>> you see
> >>>>>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to
> >>>>>>>>>>>>>>>>>>>> other sources with
> >>>>>>>>>>>>>>>>>>>> unknew
> >>>>>>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We
> >>>>>>>>>>>>>>>>>>>> allow switching to
> >>>>>>>>>>>>>>>>>>>> open-loop mode but different sources.
> >>>>>>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce
> >>>>>>>>>>>>>>>>>> DFLL freq to
> >>>>>>>>>>>>>>>>>> PLLP's
> >>>>>>>>>>>>>>>>>> rate before switching to PLLP in order to have a
> >>>>>>>>>>>>>>>>>> proper CPU voltage.
> >>>>>>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no
> >>>>>>>>>>>>>>>>> need to enforce
> >>>>>>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source
> >>>>>>>>>>>>>>>>> to PLLP during
> >>>>>>>>>>>>>>>>> suspend
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Sorry, please ignore my above comment. During
> >>>>>>>>>>>>>>>> suspend, need to change
> >>>>>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop
> >>>>>>>>>>>>>>>> mode first and
> >>>>>>>>>>>>>>>> then
> >>>>>>>>>>>>>>>> dfll need to be set to open loop.
> >>>>>>>>>>>>>>> Okay.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> And I don't exactly understand why we need to
> >>>>>>>>>>>>>>>>>>>> switch to PLLP in
> >>>>>>>>>>>>>>>>>>>> CPU
> >>>>>>>>>>>>>>>>>>>> idle
> >>>>>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the
> >>>>>>>>>>>>>>>>>>>> time.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it
> >>>>>>>>>>>>>>>>>>>> the open-loop
> >>>>>>>>>>>>>>>>>>>> mode. That's
> >>>>>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest
> >>>>>>>>>>>>>>>>>>>> of the sequence to
> >>>>>>>>>>>>>>>>>>>> turn off
> >>>>>>>>>>>>>>>>>>>> the CPU power.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the
> >>>>>>>>>>>>>>>>>>>> sequence to
> >>>>>>>>>>>>>>>>>>>> turn on
> >>>>>>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And leave
> >>>>>>>>>>>>>>>>>>>> it on PLL_P.
> >>>>>>>>>>>>>>>>>>>> After
> >>>>>>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore
> >>>>>>>>>>>>>>>>>>>> the CPU clock
> >>>>>>>>>>>>>>>>>>>> policy (CPU
> >>>>>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to
> >>>>>>>>>>>>>>>>>>>> close-loop mode.
> >>>>>>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL
> >>>>>>>>>>>>>>>>>> parent during of
> >>>>>>>>>>>>>>>>>> the
> >>>>>>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence
> >>>>>>>>>>>>>>>>>> instead of having
> >>>>>>>>>>>>>>>>>> odd
> >>>>>>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a
> >>>>>>>>>>>>>>>>>> proper suspend-resume sequencing of the device
> >>>>>>>>>>>>>>>>>> drivers. In this case
> >>>>>>>>>>>>>>>>>> CPUFreq
> >>>>>>>>>>>>>>>>>> driver is the driver that enables DFLL and switches
> >>>>>>>>>>>>>>>>>> CPU to that
> >>>>>>>>>>>>>>>>>> clock
> >>>>>>>>>>>>>>>>>> source, which means that this driver is also should
> >>>>>>>>>>>>>>>>>> be responsible for
> >>>>>>>>>>>>>>>>>> management of the DFLL's state during of
> >>>>>>>>>>>>>>>>>> suspend/resume process. If
> >>>>>>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and
> >>>>>>>>>>>>>>>>>> re-enables it
> >>>>>>>>>>>>>>>>>> during
> >>>>>>>>>>>>>>>>>> resume, then looks like the CaR driver hacks around
> >>>>>>>>>>>>>>>>>> DFLL are not
> >>>>>>>>>>>>>>>>>> needed.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the
> >>>>>>>>>>>>>>>>>>>> patch subject to
> >>>>>>>>>>>>>>>>>>>> "Add
> >>>>>>>>>>>>>>>>>>>> suspend-resume support" seems more appropriate to
> >>>>>>>>>>>>>>>>>>>> me.
> >>>>>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use are as
> >>>>>>>>>>>>>>>>>>> follows (assuming
> >>>>>>>>>>>>>>>>>>> all
> >>>>>>>>>>>>>>>>>>> required DFLL hw configuration has been done)
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> Switch to DFLL:
> >>>>>>>>>>>>>>>>>>> 0) Save current parent and frequency
> >>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
> >>>>>>>>>>>>>>>>>>> 2) Enable DFLL
> >>>>>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
> >>>>>>>>>>>>>>>>>>> For OVR regulator:
> >>>>>>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
> >>>>>>>>>>>>>>>>>>> 5) Enable DFLL PWM output
> >>>>>>>>>>>>>>>>>>> For I2C regulator:
> >>>>>>>>>>>>>>>>>>> 4) Enable DFLL I2C output
> >>>>>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> Switch away from DFLL:
> >>>>>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU
> >>>>>>>>>>>>>>>>>>> frequency is ok for
> >>>>>>>>>>>>>>>>>>> any
> >>>>>>>>>>>>>>>>>>> vdd_cpu voltage
> >>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> I see during switch away from DFLL (suspend), cclk_g
> >>>>>>>>>>>>>>>> parent is not
> >>>>>>>>>>>>>>>> changed to PLLP before changing dfll to open loop
> >>>>>>>>>>>>>>>> mode.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Will add this ...
> >>>>>>>>>>>>>>> The CPUFreq driver switches parent to PLLP during the
> >>>>>>>>>>>>>>> probe, similar
> >>>>>>>>>>>>>>> should be done on suspend.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I'm also wondering if it's always safe to switch to
> >>>>>>>>>>>>>>> PLLP in the probe.
> >>>>>>>>>>>>>>> If CPU is running on a lower freq than PLLP, then some
> >>>>>>>>>>>>>>> other more
> >>>>>>>>>>>>>>> appropriate intermediate parent should be selected.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always
> >>>>>>>>>>>>>> runs at higher
> >>>>>>>>>>>>>> rate
> >>>>>>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to
> >>>>>>>>>>>>>> dfll clock enable
> >>>>>>>>>>>>>> should be safe.
> >>>>>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a
> >>>>>>>>>>>>> divided output of
> >>>>>>>>>>>>> PLLP
> >>>>>>>>>>>>> which CCLKG supports, the PLLP_OUT4.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Probably, realistically, CPU is always running off a
> >>>>>>>>>>>>> fast PLLX during
> >>>>>>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I
> >>>>>>>>>>>>> guess ideally CPUFreq driver should also have a
> >>>>>>>>>>>>> 'shutdown' callback to teardown DFLL
> >>>>>>>>>>>>> on a reboot, but likely that there are other
> >>>>>>>>>>>>> clock-related problems as
> >>>>>>>>>>>>> well that may break KEXEC and thus it is not very
> >>>>>>>>>>>>> important at the
> >>>>>>>>>>>>> moment.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> [snip]
> >>>>>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source
> >>>>>>>>>>>> above I meant
> >>>>>>>>>>>> PLL_P_OUT4.
> >>>>>>>>>>>>
> >>>>>>>>>>>> As per clock policies, PLL_X is always used for high freq
> >>>>>>>>>>>> like
> >>>>>>>>>>>>> 800Mhz
> >>>>>>>>>>>> and for low frequency it will be sourced from PLLP.
> >>>>>>>>>>> Alright, then please don't forget to pre-initialize
> >>>>>>>>>>> PLLP_OUT4 rate to a
> >>>>>>>>>>> reasonable value using tegra_clk_init_table or
> >>>>>>>>>>> assigned-clocks.
> >>>>>>>>>> PLLP_OUT4 rate update is not needed as it is safe to run at
> >>>>>>>>>> 408Mhz because it is below fmax @ Vmin
> >>>>>>>>> So even 204MHz CVB entries are having the same voltage as
> >>>>>>>>> 408MHz, correct? It's not instantly obvious to me from the
> >>>>>>>>> DFLL driver's code where the fmax @ Vmin is defined, I see
> >>>>>>>>> that there is the min_millivolts
> >>>>>>>>> and frequency entries starting from 204MHZ defined
> >>>>>>>>> per-table.
> >>>>>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will
> >>>>>>>> work at Vmin voltage and PLLP max is 408Mhz.
> >>>>>>> Thank you for the clarification. It would be good to have that
> >>>>>>> commented
> >>>>>>> in the code as well.
> >>>>>> OK, Will add...
> >>>>> Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend
> >>>>> happens very early even before disabling non-boot CPUs and also
> >>>>> need to export clock driver APIs to CPUFreq.
> >>>>>
> >>>>> Was thinking of below way of implementing this...
> >>>>>
> >>>>>
> >>>>> Clock DFLL driver Suspend:
> >>>>>
> >>>>> - Save CPU clock policy registers, and Perform dfll
> >>>>> suspend which sets in open loop mode
> >>>>>
> >>>>> CPU Freq driver Suspend: does nothing
> >>>>>
> >>>>>
> >>>>> Clock DFLL driver Resume:
> >>>>>
> >>>>> - Re-init DFLL, Set in Open-Loop mode, restore CPU
> >>>>> Clock policy registers which actually sets source to DFLL along
> >>>>> with other CPU Policy register restore.
> >>>>>
> >>>>> CPU Freq driver Resume:
> >>>>>
> >>>>> - do clk_prepare_enable which acutally sets DFLL in
> >>>>> Closed loop mode
> >>>>>
> >>>>>
> >>>>> Adding one more note: Switching CPU Clock to PLLP is not needed
> >>>>> as CPU CLock can be from dfll in open-loop mode as DFLL is not
> >>>>> disabled anywhere throught the suspend/resume path and SC7 entry
> >>>>> FW and Warm boot code will switch CPU source to PLLP.
> >>> Since CPU resumes on PLLP, it will be cleaner to suspend it on
> >>> PLLP as well. And besides, seems that currently disabling DFLL
> >>> clock will disable DFLL completely and then you'd want to re-init
> >>> the DFLL on resume any ways. So better to just disable DFLL
> >>> completely on suspend, which should happen on clk_disable(dfll).
> >> Will switch to PLLP during CPUFreq suspend. With decision of using
> >> clk_disable during suspend, its mandatory to switch to PLLP as DFLL
> >> is completely disabled.
> >>
> >> My earlier concern was on restoring CPU policy as we can't do that
> >> from CPUFreq driver and need export from clock driver.
> >>
> >> Clear now and will do CPU clock policy restore in after dfll
> >> re-init.
> > Why the policy can't be saved/restored by the CaR driver as a
> > context of any other clock?
>
> restoring cpu clock policy involves programming source and
> super_cclkg_divider.
>
> cclk_g is registered as clk_super_mux and it doesn't use frac_div ops
> to do save/restore its divider.
That can be changed of course and I guess it also could be as simple as
saving and restoring of two raw u32 values of the policy/divider
registers.
> Also, during clock context we cant restore cclk_g as cclk_g source
> will be dfll and dfll will not be resumed/re-initialized by the time
> clk_super_mux save/restore happens.
>
> we can't use save/restore context for dfll clk_ops because
> dfllCPU_out parent to CCLK_G is first in the clock tree and dfll_ref
> and dfll_soc peripheral clocks are not restored by the time dfll
> restore happens. Also dfll peripheral clock enables need to be
> restored before dfll restore happens which involves programming dfll
> controller for re-initialization.
>
> So dfll resume/re-init is done in clk-tegra210 at end of all clocks
> restore in V5 series but instead of in clk-tegra210 driver I moved
> now to dfll-fcpu driver pm_ops as all dfll dependencies will be
> restored thru clk_restore_context by then. This will be in V6.
Since DFLL is now guaranteed to be disabled across CaR suspend/resume
(hence it has nothing to do in regards to CCLK) and given that PLLs
state is restored before the rest of the clocks, I don't see why not to
implement CCLK save/restore in a generic fasion. CPU policy wull be
restored to either PLLP or PLLX (if CPUFreq driver is disabled).
^ permalink raw reply
* [PATCH 3/3] arm64: dts: freescale: Add support for i.MX8QXP AI_ML board
From: Manivannan Sadhasivam @ 2019-07-17 6:10 UTC (permalink / raw)
To: shawnguo, s.hauer, robh+dt
Cc: kernel, festevam, linux-imx, devicetree, linux-arm-kernel,
linux-kernel, Darshak.Patel, kinjan.patel, prajose.john,
Manivannan Sadhasivam
In-Reply-To: <20190717061039.9271-1-manivannan.sadhasivam@linaro.org>
Add support for i.MX8QXP AI_ML board from Einfochips. This board is one
of the Consumer Edition boards of the 96Boards family based on i.MX8QXP
SoC from NXP/Freescale.
The initial support includes following peripherals which are tested and
known to be working:
1. Debug serial via UART2
2. uSD
3. WiFi
4. Ethernet
More information about this board can be found in Arrow website:
https://www.arrow.com/en/products/imx8-ai-ml/arrow-development-tools
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
arch/arm64/boot/dts/freescale/Makefile | 1 +
.../boot/dts/freescale/imx8qxp-ai_ml.dts | 249 ++++++++++++++++++
2 files changed, 250 insertions(+)
create mode 100644 arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
index 0bd122f60549..bd8460549d1a 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -24,4 +24,5 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb
+dtb-$(CONFIG_ARCH_MXC) += imx8qxp-ai_ml.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb
diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts b/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
new file mode 100644
index 000000000000..dcd36e57d916
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 Einfochips
+ * Copyright 2019 Linaro Ltd.
+ */
+
+/dts-v1/;
+
+#include "imx8qxp.dtsi"
+
+/ {
+ model = "Einfochips i.MX8QXP AI_ML";
+ compatible = "einfochips,imx8qxp-ai_ml", "fsl,imx8qxp";
+
+ aliases {
+ serial1 = &adma_lpuart1;
+ serial2 = &adma_lpuart2;
+ serial3 = &adma_lpuart3;
+ };
+
+ chosen {
+ stdout-path = &adma_lpuart2;
+ };
+
+ memory@80000000 {
+ device_type = "memory";
+ reg = <0x00000000 0x80000000 0 0x80000000>;
+ };
+
+ leds {
+ compatible = "gpio-leds";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_leds>;
+
+ user_led1 {
+ label = "green:user1";
+ gpios = <&lsio_gpio4 16 GPIO_ACTIVE_HIGH>;
+ linux,default-trigger = "heartbeat";
+ };
+
+ user_led2 {
+ label = "green:user2";
+ gpios = <&lsio_gpio0 6 GPIO_ACTIVE_HIGH>;
+ linux,default-trigger = "none";
+ };
+
+ user_led3 {
+ label = "green:user3";
+ gpios = <&lsio_gpio0 7 GPIO_ACTIVE_HIGH>;
+ linux,default-trigger = "mmc1";
+ default-state = "off";
+ };
+
+ user_led4 {
+ label = "green:user4";
+ gpios = <&lsio_gpio4 21 GPIO_ACTIVE_HIGH>;
+ panic-indicator;
+ linux,default-trigger = "none";
+ };
+
+ wlan_active_led {
+ label = "yellow:wlan";
+ gpios = <&lsio_gpio4 17 GPIO_ACTIVE_HIGH>;
+ linux,default-trigger = "phy0tx";
+ default-state = "off";
+ };
+
+ bt_active_led {
+ label = "blue:bt";
+ gpios = <&lsio_gpio4 18 GPIO_ACTIVE_HIGH>;
+ linux,default-trigger = "hci0-power";
+ default-state = "off";
+ };
+ };
+
+ sdio_pwrseq: sdio-pwrseq {
+ compatible = "mmc-pwrseq-simple";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_wifi_reg_on>;
+ reset-gpios = <&lsio_gpio3 24 GPIO_ACTIVE_LOW>;
+ };
+};
+
+/* BT */
+&adma_lpuart0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_lpuart0>;
+ uart-has-rtscts;
+ status = "okay";
+};
+
+/* LS-I2C0 */
+&adma_lpuart1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_lpuart1>;
+ status = "okay";
+};
+
+/* Debug */
+&adma_lpuart2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_lpuart2>;
+ status = "okay";
+};
+
+/* PCI-E */
+&adma_lpuart3 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_lpuart3>;
+ status = "okay";
+};
+
+&fec1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_fec1>;
+ phy-mode = "rgmii-id";
+ phy-handle = <ðphy0>;
+ fsl,magic-packet;
+ status = "okay";
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ethphy0: ethernet-phy@0 {
+ compatible = "ethernet-phy-ieee802.3-c22";
+ reg = <0>;
+ };
+ };
+};
+
+/* WiFi */
+&usdhc1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usdhc1>;
+ bus-width = <4>;
+ no-sd;
+ non-removable;
+ mmc-pwrseq = <&sdio_pwrseq>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "okay";
+
+ brcmf: wifi@1 {
+ reg = <1>;
+ compatible = "brcm,bcm4329-fmac";
+ };
+};
+
+/* SD */
+&usdhc2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usdhc2>;
+ bus-width = <4>;
+ cd-gpios = <&lsio_gpio4 22 GPIO_ACTIVE_LOW>;
+ status = "okay";
+};
+
+&iomuxc {
+ pinctrl_fec1: fec1grp {
+ fsl,pins = <
+ IMX8QXP_ENET0_MDC_CONN_ENET0_MDC 0x06000020
+ IMX8QXP_ENET0_MDIO_CONN_ENET0_MDIO 0x06000020
+ IMX8QXP_ENET0_RGMII_TX_CTL_CONN_ENET0_RGMII_TX_CTL 0x06000020
+ IMX8QXP_ENET0_RGMII_TXC_CONN_ENET0_RGMII_TXC 0x06000020
+ IMX8QXP_ENET0_RGMII_TXD0_CONN_ENET0_RGMII_TXD0 0x06000020
+ IMX8QXP_ENET0_RGMII_TXD1_CONN_ENET0_RGMII_TXD1 0x06000020
+ IMX8QXP_ENET0_RGMII_TXD2_CONN_ENET0_RGMII_TXD2 0x06000020
+ IMX8QXP_ENET0_RGMII_TXD3_CONN_ENET0_RGMII_TXD3 0x06000020
+ IMX8QXP_ENET0_RGMII_RXC_CONN_ENET0_RGMII_RXC 0x06000020
+ IMX8QXP_ENET0_RGMII_RX_CTL_CONN_ENET0_RGMII_RX_CTL 0x06000020
+ IMX8QXP_ENET0_RGMII_RXD0_CONN_ENET0_RGMII_RXD0 0x06000020
+ IMX8QXP_ENET0_RGMII_RXD1_CONN_ENET0_RGMII_RXD1 0x06000020
+ IMX8QXP_ENET0_RGMII_RXD2_CONN_ENET0_RGMII_RXD2 0x06000020
+ IMX8QXP_ENET0_RGMII_RXD3_CONN_ENET0_RGMII_RXD3 0x06000020
+ >;
+ };
+
+ pinctrl_leds: ledsgrp{
+ fsl,pins = <
+ IMX8QXP_ESAI0_TX2_RX3_LSIO_GPIO0_IO06 0x00000021
+ IMX8QXP_ESAI0_TX3_RX2_LSIO_GPIO0_IO07 0x00000021
+ IMX8QXP_EMMC0_DATA7_LSIO_GPIO4_IO16 0x00000021
+ IMX8QXP_USDHC1_WP_LSIO_GPIO4_IO21 0x00000021
+ IMX8QXP_EMMC0_STROBE_LSIO_GPIO4_IO17 0x00000021
+ IMX8QXP_EMMC0_RESET_B_LSIO_GPIO4_IO18 0x00000021
+ >;
+ };
+
+ pinctrl_lpuart0: lpuart0grp {
+ fsl,pins = <
+ IMX8QXP_UART0_RX_ADMA_UART0_RX 0X06000020
+ IMX8QXP_UART0_TX_ADMA_UART0_TX 0X06000020
+ IMX8QXP_FLEXCAN0_TX_ADMA_UART0_CTS_B 0x06000020
+ IMX8QXP_FLEXCAN0_RX_ADMA_UART0_RTS_B 0x06000020
+ >;
+ };
+
+ pinctrl_lpuart1: lpuart1grp {
+ fsl,pins = <
+ IMX8QXP_UART1_RX_ADMA_UART1_RX 0X06000020
+ IMX8QXP_UART1_TX_ADMA_UART1_TX 0X06000020
+ >;
+ };
+
+ pinctrl_lpuart2: lpuart2grp {
+ fsl,pins = <
+ IMX8QXP_UART2_RX_ADMA_UART2_RX 0X06000020
+ IMX8QXP_UART2_TX_ADMA_UART2_TX 0X06000020
+ >;
+ };
+
+ pinctrl_lpuart3: lpuart3grp {
+ fsl,pins = <
+ IMX8QXP_FLEXCAN2_RX_ADMA_UART3_RX 0X06000020
+ IMX8QXP_FLEXCAN2_TX_ADMA_UART3_TX 0X06000020
+ >;
+ };
+
+ pinctrl_usdhc1: usdhc1grp {
+ fsl,pins = <
+ IMX8QXP_EMMC0_CLK_CONN_EMMC0_CLK 0x06000041
+ IMX8QXP_EMMC0_CMD_CONN_EMMC0_CMD 0x00000021
+ IMX8QXP_EMMC0_DATA0_CONN_EMMC0_DATA0 0x00000021
+ IMX8QXP_EMMC0_DATA1_CONN_EMMC0_DATA1 0x00000021
+ IMX8QXP_EMMC0_DATA2_CONN_EMMC0_DATA2 0x00000021
+ IMX8QXP_EMMC0_DATA3_CONN_EMMC0_DATA3 0x00000021
+ >;
+ };
+
+ pinctrl_usdhc2: usdhc2grp {
+ fsl,pins = <
+ IMX8QXP_USDHC1_CLK_CONN_USDHC1_CLK 0x06000041
+ IMX8QXP_USDHC1_CMD_CONN_USDHC1_CMD 0x00000021
+ IMX8QXP_USDHC1_DATA0_CONN_USDHC1_DATA0 0x00000021
+ IMX8QXP_USDHC1_DATA1_CONN_USDHC1_DATA1 0x00000021
+ IMX8QXP_USDHC1_DATA2_CONN_USDHC1_DATA2 0x00000021
+ IMX8QXP_USDHC1_DATA3_CONN_USDHC1_DATA3 0x00000021
+ IMX8QXP_USDHC1_VSELECT_CONN_USDHC1_VSELECT 0x00000021
+ IMX8QXP_USDHC1_CD_B_LSIO_GPIO4_IO22 0x00000021
+ >;
+ };
+
+ pinctrl_wifi_reg_on: wifiregongrp {
+ fsl,pins = <
+ IMX8QXP_QSPI0B_SS1_B_LSIO_GPIO3_IO24 0x00000021
+ >;
+ };
+};
--
2.17.1
^ permalink raw reply related
* [PATCH 2/3] dt-bindings: arm: Document i.MX8QXP AI_ML board binding
From: Manivannan Sadhasivam @ 2019-07-17 6:10 UTC (permalink / raw)
To: shawnguo, s.hauer, robh+dt
Cc: kernel, festevam, linux-imx, devicetree, linux-arm-kernel,
linux-kernel, Darshak.Patel, kinjan.patel, prajose.john,
Manivannan Sadhasivam
In-Reply-To: <20190717061039.9271-1-manivannan.sadhasivam@linaro.org>
Document devicetree binding of i.MX8QXP AI_ML board from Einfochips.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Documentation/devicetree/bindings/arm/fsl.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
index 407138ebc0d0..8e9209a75478 100644
--- a/Documentation/devicetree/bindings/arm/fsl.yaml
+++ b/Documentation/devicetree/bindings/arm/fsl.yaml
@@ -180,6 +180,7 @@ properties:
- description: i.MX8QXP based Boards
items:
- enum:
+ - einfochips,imx8qxp-ai_ml # i.MX8QXP AI_ML Board
- fsl,imx8qxp-mek # i.MX8QXP MEK Board
- const: fsl,imx8qxp
--
2.17.1
^ permalink raw reply related
* [PATCH 1/3] dt-bindings: Add Vendor prefix for Einfochips
From: Manivannan Sadhasivam @ 2019-07-17 6:10 UTC (permalink / raw)
To: shawnguo, s.hauer, robh+dt
Cc: kernel, festevam, linux-imx, devicetree, linux-arm-kernel,
linux-kernel, Darshak.Patel, kinjan.patel, prajose.john,
Manivannan Sadhasivam
In-Reply-To: <20190717061039.9271-1-manivannan.sadhasivam@linaro.org>
Add devicetree vendor prefix for Einfochips.
https://www.einfochips.com/
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 1acf806b62bf..9b74c4de5676 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -255,6 +255,8 @@ patternProperties:
description: Emerging Display Technologies
"^eeti,.*":
description: eGalax_eMPIA Technology Inc
+ "^einfochips,.*":
+ description: Einfochips
"^elan,.*":
description: Elan Microelectronic Corp.
"^elgin,.*":
--
2.17.1
^ permalink raw reply related
* [PATCH 0/3] Add support for i.MXQXP AI_ML board
From: Manivannan Sadhasivam @ 2019-07-17 6:10 UTC (permalink / raw)
To: shawnguo, s.hauer, robh+dt
Cc: devicetree, kinjan.patel, linux-kernel, Darshak.Patel, linux-imx,
kernel, Manivannan Sadhasivam, festevam, linux-arm-kernel,
prajose.john
Hello,
This patchset adds support for i.MXQXP AI_ML board from Einfochips.
This board is one of the Consumer Edition boards of the 96Boards family
based on i.MX8QXP SoC from NXP/Freescale.
The initial support includes following peripherals which are tested and
known to be working:
1. Debug serial via UART2
2. uSD
3. WiFi
4. Ethernet
More information about this board can be found in Arrow website:
https://www.arrow.com/en/products/imx8-ai-ml/arrow-development-tools
Thanks,
Mani
Manivannan Sadhasivam (3):
dt-bindings: Add Vendor prefix for Einfochips
dt-bindings: arm: Document i.MX8QXP AI_ML board binding
arm64: dts: freescale: Add support for i.MX8QXP AI_ML board
.../devicetree/bindings/arm/fsl.yaml | 1 +
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
arch/arm64/boot/dts/freescale/Makefile | 1 +
.../boot/dts/freescale/imx8qxp-ai_ml.dts | 249 ++++++++++++++++++
4 files changed, 253 insertions(+)
create mode 100644 arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
--
2.17.1
^ permalink raw reply
* Re: [PATCH v4, 22/33] drm/mediatek: add background color input select function for ovl/ovl_2l
From: CK Hu @ 2019-07-17 5:58 UTC (permalink / raw)
To: yongqiang.niu
Cc: Philipp Zabel, Rob Herring, Matthias Brugger, David Airlie,
Daniel Vetter, Mark Rutland, dri-devel, devicetree, linux-kernel,
linux-arm-kernel, linux-mediatek
In-Reply-To: <1562625253-29254-23-git-send-email-yongqiang.niu@mediatek.com>
Hi, Yongqiang:
On Tue, 2019-07-09 at 06:34 +0800, yongqiang.niu@mediatek.com wrote:
> From: Yongqiang Niu <yongqiang.niu@mediatek.com>
>
> This patch add background color input select function for ovl/ovl_2l
>
> ovl include 4 DRAM layer and 1 background color layer
> ovl_2l include 4 DRAM layer and 1 background color layer
> DRAM layer frame buffer data from render hardware, GPU for example.
> backgournd color layer is embed in ovl/ovl_2l, we can only set
> it color, but not support DRAM frame buffer.
>
> for ovl0->ovl0_2l direct link usecase,
> we need set ovl0_2l background color intput select from ovl0
> if render send DRAM buffer layer number <=4, all these layer read
> by ovl.
> layer0 is at the bottom of all layers.
> layer3 is at the top of all layers.
> if render send DRAM buffer layer numbfer >=4 && <=6
> ovl0 read layer0~3
> ovl0_2l read layer4~5
> layer5 is at the top ot all these layers.
>
> the decision of how to setting ovl0/ovl0_2l read these layer data
> is controlled in mtk crtc, which will be another patch
>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index a0ab760..8ca4965 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -27,6 +27,8 @@
> #define DISP_REG_OVL_EN 0x000c
> #define DISP_REG_OVL_RST 0x0014
> #define DISP_REG_OVL_ROI_SIZE 0x0020
> +#define DISP_REG_OVL_DATAPATH_CON 0x0024
> +#define OVL_BGCLR_SEL_IN BIT(2)
> #define DISP_REG_OVL_ROI_BGCLR 0x0028
> #define DISP_REG_OVL_SRC_CON 0x002c
> #define DISP_REG_OVL_CON(n) (0x0030 + 0x20 * (n))
> @@ -245,6 +247,24 @@ static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx,
> mtk_ovl_layer_on(comp, idx);
> }
>
> +static void mtk_ovl_bgclr_in_on(struct mtk_ddp_comp *comp)
> +{
> + unsigned int reg;
> +
> + reg = readl(comp->regs + DISP_REG_OVL_DATAPATH_CON);
> + reg = reg | OVL_BGCLR_SEL_IN;
> + writel(reg, comp->regs + DISP_REG_OVL_DATAPATH_CON);
> +}
> +
> +static void mtk_ovl_bgclr_in_off(struct mtk_ddp_comp *comp)
> +{
> + unsigned int reg;
> +
> + reg = readl(comp->regs + DISP_REG_OVL_DATAPATH_CON);
> + reg = reg & ~OVL_BGCLR_SEL_IN;
> + writel(reg, comp->regs + DISP_REG_OVL_DATAPATH_CON);
> +}
> +
> static const struct mtk_ddp_comp_funcs mtk_disp_ovl_funcs = {
> .config = mtk_ovl_config,
> .start = mtk_ovl_start,
> @@ -255,6 +275,8 @@ static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx,
> .layer_on = mtk_ovl_layer_on,
> .layer_off = mtk_ovl_layer_off,
> .layer_config = mtk_ovl_layer_config,
> + .bgclr_in_on = mtk_ovl_bgclr_in_on,
> + .bgclr_in_off = mtk_ovl_bgclr_in_off,
> };
>
> static int mtk_disp_ovl_bind(struct device *dev, struct device *master,
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox