* Re: [PATCH v5 1/2] arm64: dts: fsl: pico-pi: Add a device tree for the PICO-PI-IMX8M
From: Fabio Estevam @ 2019-07-20 15:09 UTC (permalink / raw)
To: andradanciu1997
Cc: Shawn Guo, Rob Herring, Mark Rutland, Sascha Hauer, Sascha Hauer,
NXP Linux Team, Manivannan Sadhasivam, Andrey Smirnov,
Jonathan Neuschäfer, Dong Aisheng, Li Yang, Lucas Stach,
pankaj.bansal, Bhaskar Upadhaya, Pramod Kumar,
Angus Ainslie (Purism), Richard Hu,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <20190719121430.9318-2-andradanciu1997@gmail.com>
Hi Andra,
Just realized one minor issue:
On Fri, Jul 19, 2019 at 9:14 AM andradanciu1997
<andradanciu1997@gmail.com> wrote:
> +&i2c1 {
> + clock-frequency = <100000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_i2c1>;
> + status = "okay";
> +
> + pmic: pmic@4b {
> + reg = <0x4b>;
> + compatible = "rohm,bd71837";
> + /* PMIC BD71837 PMIC_nINT GPIO1_IO12 */
Comment says gpio1 12...
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_pmic>;
> + clocks = <&pmic_osc>;
> + clock-names = "osc";
> + clock-output-names = "pmic_clk";
> + interrupt-parent = <&gpio1>;
> + interrupts = <3 GPIO_ACTIVE_LOW>;
but here you use gpio1 3 instead, so there is a mismatch.
Please check against the schematics and pick the correct one.
I would suggest removing the:
/* PMIC BD71837 PMIC_nINT GPIO1_IO12 */
comment entirely.
For the next version you can:
Reviewed-by: Fabio Estevam <festevam@gmail.com>
Thanks
^ permalink raw reply
* Re: [PATCH v1 2/2] auxadc: mediatek: support efuse calibration in auxadc driver
From: Nicolas Boichat @ 2019-07-20 14:13 UTC (permalink / raw)
To: Zhiyong Tao
Cc: Rob Herring, jic23, knaack.h, Lars-Peter Clausen, pmeerw,
srv_heupstream, hui.liu, jg_poxu, sj.huang, Yingjoe Chen,
Sean Wang, Erin Lo, Eddie Huang, Matthias Brugger, Sascha Hauer,
devicetree, lkml, linux-arm Mailing List, linux-iio,
moderated list:ARM/Mediatek SoC support
In-Reply-To: <20190718080119.30707-3-zhiyong.tao@mediatek.com>
On Thu, Jul 18, 2019 at 4:01 PM Zhiyong Tao <zhiyong.tao@mediatek.com> wrote:
>
> This patch support efuse calibration in auxadc driver
>
> Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
> Signed-off-by: jg_poxu <jg_poxu@mediatek.com>
> ---
> drivers/iio/adc/mt6577_auxadc.c | 71 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 71 insertions(+)
>
> diff --git a/drivers/iio/adc/mt6577_auxadc.c b/drivers/iio/adc/mt6577_auxadc.c
> index 95d76abb64ec..e30d9736b1a5 100644
> --- a/drivers/iio/adc/mt6577_auxadc.c
> +++ b/drivers/iio/adc/mt6577_auxadc.c
> @@ -17,12 +17,14 @@
> #include <linux/err.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/iopoll.h>
> #include <linux/io.h>
> #include <linux/iio/iio.h>
> +#include <linux/slab.h>
>
> /* Register definitions */
> #define MT6577_AUXADC_CON0 0x00
> @@ -42,6 +44,13 @@
> #define MT6577_AUXADC_POWER_READY_MS 1
> #define MT6577_AUXADC_SAMPLE_READY_US 25
>
> +#define ADC_GE_A_SHIFT 10
> +#define ADC_GE_A_MASK (0x3ff << ADC_GE_A_SHIFT)
> +#define ADC_OE_A_SHIFT 0
> +#define ADC_OE_A_MASK (0x3ff << ADC_OE_A_SHIFT)
> +#define ADC_CALI_EN_A_SHIFT 20
> +#define ADC_CALI_EN_A_MASK (0x1 << ADC_CALI_EN_A_SHIFT)
> +
> struct mt6577_auxadc_device {
> void __iomem *reg_base;
> struct clk *adc_clk;
> @@ -74,6 +83,64 @@ static const struct iio_chan_spec mt6577_auxadc_iio_channels[] = {
> MT6577_AUXADC_CHANNEL(15),
> };
>
> +s32 cali_oe;
> +s32 cali_ge;
Why do you need these 2 (global) variables? They are set in
mt6577_auxadc_update_cali but never read anywhere.
> +struct adc_cali_info {
> + u32 cali_ge_a;
> + u32 cali_oe_a;
> + u32 gain;
> +};
> +static struct adc_cali_info adc_cali;
This should be inside struct mt6577_auxadc_device structure. And... I
still do not understand what this does, since nothing reads back
adc_cali either...
On gerrit (https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1624410),
you mentioned that thermal module will use these, can you please
upload these changes as part of this series, so that we can review the
whole thing in one piece (and understand better how to fix this
interface).
> +
> +static int mt6577_auxadc_update_cali(struct device *dev)
> +{
> + struct nvmem_cell *cell;
> + u32 *buf;
> + size_t len;
> + int ret = 0;
> +
> + cali_oe = 0;
> + cali_ge = 0;
> +
> + cell = nvmem_cell_get(dev, "calibration-data");
> + if (IS_ERR(cell)) {
> + if (PTR_ERR(cell) == -EPROBE_DEFER)
> + return PTR_ERR(cell);
> + return 0;
> + }
> +
> + buf = (u32 *)nvmem_cell_read(cell, &len);
Cast is not necessary.
> +
> + nvmem_cell_put(cell);
> +
> + if (IS_ERR(buf))
> + return PTR_ERR(buf);
> +
> + if (len < sizeof(u32)) {
> + dev_warn(dev, "invalid calibration data\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (((buf[0] & ADC_CALI_EN_A_MASK) >> ADC_CALI_EN_A_SHIFT) != 0) {
> + adc_cali.cali_oe_a =
> + (buf[0] & ADC_OE_A_MASK) >> ADC_OE_A_SHIFT;
> + adc_cali.cali_ge_a =
> + ((buf[0] & ADC_GE_A_MASK) >> ADC_GE_A_SHIFT);
> +
> + cali_ge = adc_cali.cali_ge_a - 512;
> + cali_oe = adc_cali.cali_oe_a - 512;
> + adc_cali.gain = 1 + cali_ge;
> + } else {
> + dev_info(dev, "Device not calibrated, using default calibration values\n");
> + }
> +
> +out:
> + kfree(buf);
> +
> + return ret;
> +}
> +
> static inline void mt6577_auxadc_mod_reg(void __iomem *reg,
> u32 or_mask, u32 and_mask)
> {
> @@ -274,6 +341,10 @@ static int mt6577_auxadc_probe(struct platform_device *pdev)
> goto err_power_off;
> }
>
> + ret = mt6577_auxadc_update_cali(&pdev->dev);
> + if (ret)
> + return ret;
> +
> return 0;
Or just
return mt6577_auxadc_update_cali(&pdev->dev);
>
> err_power_off:
> --
> 2.12.5
>
^ permalink raw reply
* Re: [Question] orphan platform data header
From: Arnd Bergmann @ 2019-07-20 13:54 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Linux Kernel Mailing List, Linus Torvalds, Greg Kroah-Hartman,
DTML, linux-arm-kernel, masahiroy
In-Reply-To: <CAK7LNASyzmYjjBkFxRc06rqf36-en-bvJvrKcg6iiRfjoPCxhQ@mail.gmail.com>
On Sat, Jul 20, 2019 at 5:26 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> masahiro@grover:~/ref/linux$ git grep netxbig_led_platform_data
> drivers/leds/leds-netxbig.c: struct
> netxbig_led_platform_data *pdata,
> drivers/leds/leds-netxbig.c: struct
> netxbig_led_platform_data *pdata)
> drivers/leds/leds-netxbig.c: struct
> netxbig_led_platform_data *pdata)
> drivers/leds/leds-netxbig.c: struct netxbig_led_platform_data
> *pdata = dev_get_platdata(&pdev->dev);
> include/linux/platform_data/leds-kirkwood-netxbig.h:struct
> netxbig_led_platform_data {
>
>
>
> So, what shall we do?
>
> Drop the board-file support? Or, keep it
> in case somebody is still using their board-files
> in downstream?
Generally speaking, I'd remove the board file support in another
case like this, but it's worth looking at when it was last used and by
what.
For this file, all boards got converted to DT, and the old setup
code removed in commit ebc278f15759 ("ARM: mvebu: remove static
LED setup for netxbig boards"), four years ago, so it's a fairly
easy decision to make it DT only.
Arnd
^ permalink raw reply
* Re: [PATCH v4 7/8] dt-bindings: arm: sunxi: add binding for Lichee Zero Plus core board
From: Icenowy Zheng @ 2019-07-20 11:39 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Maxime Ripard
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Linus Walleij,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA, Chen-Yu Tsai, Rob Herring,
linux-clk-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20190720101318.cwrvv5r42wxx5k4r@flea>
于 2019年7月20日 GMT+08:00 下午6:13:18, Maxime Ripard <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> 写到:
>On Sat, Jul 13, 2019 at 11:46:33AM +0800, Icenowy Zheng wrote:
>> The Lichee Zero Plus is a core board made by Sipeed, with a microUSB
>> connector on it, TF slot or WSON8 SD chip, optional eMMC or SPI
>Flash.
>> It has a gold finger connector for expansion, and UART is available
>from
>> reserved pins w/ 2.54mm pitch. The board can use either SoChip S3 or
>> Allwinner V3L SoCs.
>>
>> Add the device tree binding of the basic version of the core board --
>> w/o eMMC or SPI Flash, w/ TF slot or WSON8 SD, and use S3 SoC.
>>
>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> ---
>> No changes since v3.
>>
>> Patch introduced in v2.
>>
>> Documentation/devicetree/bindings/arm/sunxi.yaml | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml
>b/Documentation/devicetree/bindings/arm/sunxi.yaml
>> index 000a00d12d6a..48c126a7a848 100644
>> --- a/Documentation/devicetree/bindings/arm/sunxi.yaml
>> +++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
>> @@ -353,6 +353,11 @@ properties:
>> - const: licheepi,licheepi-zero
>> - const: allwinner,sun8i-v3s
>>
>> + - description: Lichee Zero Plus (with S3, without eMMC/SPI
>Flash)
>> + items:
>> + - const: sipeed,lichee-zero-plus
>> + - const: allwinner,sun8i-s3
>
>If the S3 is just a rebranded V3, then we should have the v3 compatile
>in that list too.
S3 is V3 with copackaged DDR3 DRAM.
It's pin incompatible w/ V3.
>
>Maxime
>
>--
>Maxime Ripard, Bootlin
>Embedded Linux and Kernel engineering
>https://bootlin.com
--
使用 K-9 Mail 发送自我的Android设备。
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/BDF0C9F6-DD0D-4343-8E24-06A07055004C%40aosc.io.
^ permalink raw reply
* Re: [PATCH v4 7/8] dt-bindings: arm: sunxi: add binding for Lichee Zero Plus core board
From: Maxime Ripard @ 2019-07-20 10:13 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Rob Herring, Chen-Yu Tsai, Linus Walleij,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20190713034634.44585-8-icenowy-h8G6r0blFSE@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]
On Sat, Jul 13, 2019 at 11:46:33AM +0800, Icenowy Zheng wrote:
> The Lichee Zero Plus is a core board made by Sipeed, with a microUSB
> connector on it, TF slot or WSON8 SD chip, optional eMMC or SPI Flash.
> It has a gold finger connector for expansion, and UART is available from
> reserved pins w/ 2.54mm pitch. The board can use either SoChip S3 or
> Allwinner V3L SoCs.
>
> Add the device tree binding of the basic version of the core board --
> w/o eMMC or SPI Flash, w/ TF slot or WSON8 SD, and use S3 SoC.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> No changes since v3.
>
> Patch introduced in v2.
>
> Documentation/devicetree/bindings/arm/sunxi.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml b/Documentation/devicetree/bindings/arm/sunxi.yaml
> index 000a00d12d6a..48c126a7a848 100644
> --- a/Documentation/devicetree/bindings/arm/sunxi.yaml
> +++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
> @@ -353,6 +353,11 @@ properties:
> - const: licheepi,licheepi-zero
> - const: allwinner,sun8i-v3s
>
> + - description: Lichee Zero Plus (with S3, without eMMC/SPI Flash)
> + items:
> + - const: sipeed,lichee-zero-plus
> + - const: allwinner,sun8i-s3
If the S3 is just a rebranded V3, then we should have the v3 compatile
in that list too.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v4 8/8] ARM: dts: sun8i: s3: add devicetree for Lichee zero plus w/ S3
From: Maxime Ripard @ 2019-07-20 10:12 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Rob Herring, Chen-Yu Tsai, Linus Walleij,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20190713034634.44585-9-icenowy-h8G6r0blFSE@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3397 bytes --]
On Sat, Jul 13, 2019 at 11:46:34AM +0800, Icenowy Zheng wrote:
> Lichee zero plus is a core board made by Sipeed, which includes on-board
> TF slot or SMT SD NAND, and optional SPI NOR or eMMC, a UART debug
> header, a microUSB slot and a gold finger connector for expansion. It
> can use either Sochip S3 or Allwinner S3L SoC.
>
> Add the basic device tree for the core board, w/o optional onboard
> storage, and with S3 SoC.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> No changes in v4.
>
> Changes in v3:
> - Drop common regulator DTSI usage and added vcc3v3 regulator.
>
> arch/arm/boot/dts/Makefile | 1 +
> .../boot/dts/sun8i-s3-lichee-zero-plus.dts | 8 ++++
> .../dts/sun8i-s3-s3l-lichee-zero-plus.dtsi | 46 +++++++++++++++++++
> 3 files changed, 55 insertions(+)
> create mode 100644 arch/arm/boot/dts/sun8i-s3-lichee-zero-plus.dts
> create mode 100644 arch/arm/boot/dts/sun8i-s3-s3l-lichee-zero-plus.dtsi
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 9159fa2cea90..e320460a952b 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1114,6 +1114,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
> sun8i-r16-nintendo-super-nes-classic.dtb \
> sun8i-r16-parrot.dtb \
> sun8i-r40-bananapi-m2-ultra.dtb \
> + sun8i-s3-lichee-zero-plus.dtb \
> sun8i-t3-cqa3t-bv3.dtb \
> sun8i-v3s-licheepi-zero.dtb \
> sun8i-v3s-licheepi-zero-dock.dtb \
> diff --git a/arch/arm/boot/dts/sun8i-s3-lichee-zero-plus.dts b/arch/arm/boot/dts/sun8i-s3-lichee-zero-plus.dts
> new file mode 100644
> index 000000000000..7d2f6b145190
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun8i-s3-lichee-zero-plus.dts
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2019 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + */
> +
> +/dts-v1/;
> +#include "sun8i-s3.dtsi"
> +#include "sun8i-s3-s3l-lichee-zero-plus.dtsi"
Why do we need to mention both the S3 and S3l in the name? What are
the differences between the two, and why do you need to share a DTSI?
> diff --git a/arch/arm/boot/dts/sun8i-s3-s3l-lichee-zero-plus.dtsi b/arch/arm/boot/dts/sun8i-s3-s3l-lichee-zero-plus.dtsi
> new file mode 100644
> index 000000000000..2677d319fb29
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun8i-s3-s3l-lichee-zero-plus.dtsi
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2019 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + */
> +
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/ {
> + aliases {
> + serial0 = &uart0;
> + };
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + reg_vcc3v3: vcc3v3 {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc3v3";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + };
> +};
> +
> +&mmc0 {
> + broken-cd;
> + bus-width = <4>;
> + vmmc-supply = <®_vcc3v3>;
> + status = "okay";
> +};
> +
> +&uart0 {
> + pinctrl-0 = <&uart0_pb_pins>;
> + pinctrl-names = "default";
> + status = "okay";
> +};
> +
> +&usb_otg {
> + dr_mode = "otg";
> + status = "okay";
> +};
> +
> +&usbphy {
> + usb0_id_det-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>;
> + status = "okay";
How can it do OTG if there's no controlable VBUS?
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [RFC,v3 7/9] media: platform: Add Mediatek ISP P1 device driver
From: Jungo Lin @ 2019-07-20 9:58 UTC (permalink / raw)
To: Tomasz Figa
Cc: devicetree, sean.cheng, frederic.chen, rynn.wu, srv_heupstream,
robh, ryan.yu, frankie.chiu, hverkuil, ddavenport, sj.huang,
linux-mediatek, laurent.pinchart, matthias.bgg, mchehab,
linux-arm-kernel, linux-media
In-Reply-To: <20190710095646.GB181405@chromium.org>
Hi, Tomasz:
On Wed, 2019-07-10 at 18:56 +0900, Tomasz Figa wrote:
> Hi Jungo,
>
> On Tue, Jun 11, 2019 at 11:53:42AM +0800, Jungo Lin wrote:
> > This patch adds the Mediatek ISP P1 HW control device driver.
> > It handles the ISP HW configuration, provides interrupt handling and
> > initializes the V4L2 device nodes and other functions.
> >
> > (The current metadata interface used in meta input and partial
> > meta nodes is only a temporary solution to kick off the driver
> > development and is not ready to be reviewed yet.)
> >
> > Signed-off-by: Jungo Lin <jungo.lin@mediatek.com>
> > ---
> > .../platform/mtk-isp/isp_50/cam/Makefile | 1 +
> > .../mtk-isp/isp_50/cam/mtk_cam-regs.h | 126 ++
> > .../platform/mtk-isp/isp_50/cam/mtk_cam.c | 1087 +++++++++++++++++
> > .../platform/mtk-isp/isp_50/cam/mtk_cam.h | 243 ++++
> > 4 files changed, 1457 insertions(+)
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.h
> >
>
> Thanks for the patch! Please see my comments inline.
>
> [snip]
>
Thanks for your comments. Please check my replies inline.
[snip]
> > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h
> > new file mode 100644
> > index 000000000000..9e59a6bfc6b7
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h
> > @@ -0,0 +1,126 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + */
> > +
> > +#ifndef _CAM_REGS_H
> > +#define _CAM_REGS_H
> > +
> > +/* TG Bit Mask */
> > +#define VFDATA_EN_BIT BIT(0)
> > +#define CMOS_EN_BIT BIT(0)
> > +
> > +/* normal signal bit */
> > +#define VS_INT_ST BIT(0)
> > +#define HW_PASS1_DON_ST BIT(11)
> > +#define SOF_INT_ST BIT(12)
> > +#define SW_PASS1_DON_ST BIT(30)
> > +
> > +/* err status bit */
> > +#define TG_ERR_ST BIT(4)
> > +#define TG_GBERR_ST BIT(5)
> > +#define CQ_CODE_ERR_ST BIT(6)
> > +#define CQ_APB_ERR_ST BIT(7)
> > +#define CQ_VS_ERR_ST BIT(8)
> > +#define AMX_ERR_ST BIT(15)
> > +#define RMX_ERR_ST BIT(16)
> > +#define BMX_ERR_ST BIT(17)
> > +#define RRZO_ERR_ST BIT(18)
> > +#define AFO_ERR_ST BIT(19)
> > +#define IMGO_ERR_ST BIT(20)
> > +#define AAO_ERR_ST BIT(21)
> > +#define PSO_ERR_ST BIT(22)
> > +#define LCSO_ERR_ST BIT(23)
> > +#define BNR_ERR_ST BIT(24)
> > +#define LSCI_ERR_ST BIT(25)
> > +#define DMA_ERR_ST BIT(29)
> > +
> > +/* CAM DMA done status */
> > +#define FLKO_DONE_ST BIT(4)
> > +#define AFO_DONE_ST BIT(5)
> > +#define AAO_DONE_ST BIT(7)
> > +#define PSO_DONE_ST BIT(14)
> > +
> > +/* IRQ signal mask */
> > +#define INT_ST_MASK_CAM ( \
> > + VS_INT_ST |\
> > + SOF_INT_ST |\
> > + HW_PASS1_DON_ST |\
> > + SW_PASS1_DON_ST)
> > +
> > +/* IRQ Error Mask */
> > +#define INT_ST_MASK_CAM_ERR ( \
> > + TG_ERR_ST |\
> > + TG_GBERR_ST |\
> > + CQ_CODE_ERR_ST |\
> > + CQ_APB_ERR_ST |\
> > + CQ_VS_ERR_ST |\
> > + BNR_ERR_ST |\
> > + RMX_ERR_ST |\
> > + BMX_ERR_ST |\
> > + BNR_ERR_ST |\
> > + LSCI_ERR_ST |\
> > + DMA_ERR_ST)
> > +
> > +/* IRQ Signal Log Mask */
> > +#define INT_ST_LOG_MASK_CAM ( \
> > + SOF_INT_ST |\
> > + SW_PASS1_DON_ST |\
> > + HW_PASS1_DON_ST |\
> > + VS_INT_ST |\
> > + TG_ERR_ST |\
> > + TG_GBERR_ST |\
> > + RRZO_ERR_ST |\
> > + AFO_ERR_ST |\
> > + IMGO_ERR_ST |\
> > + AAO_ERR_ST |\
> > + DMA_ERR_ST)
> > +
> > +/* DMA Event Notification Mask */
> > +#define DMA_ST_MASK_CAM ( \
> > + AAO_DONE_ST |\
> > + AFO_DONE_ST)
>
> Could we define the values next to the addresses of registers they apply to?
> Also without the _BIT suffix and with the values prefixed with register
> names. For example:
>
> #define REG_TG_SEN_MODE 0x0230
> #define TG_SEN_MODE_CMOS_EN BIT(0)
>
> #define REG_TG_VF_CON 0x0234
> #define TG_VF_CON_VFDATA_EN BIT(0)
>
Fix in next patch.
> > +
> > +/* Status check */
> > +#define REG_CTL_EN 0x0004
> > +#define REG_CTL_DMA_EN 0x0008
> > +#define REG_CTL_FMT_SEL 0x0010
> > +#define REG_CTL_EN2 0x0018
> > +#define REG_CTL_RAW_INT_EN 0x0020
> > +#define REG_CTL_RAW_INT_STAT 0x0024
> > +#define REG_CTL_RAW_INT2_STAT 0x0034
> > +
> > +#define REG_TG_SEN_MODE 0x0230
> > +#define REG_TG_VF_CON 0x0234
> > +
> > +#define REG_IMGO_BASE_ADDR 0x1020
> > +#define REG_RRZO_BASE_ADDR 0x1050
> > +
> > +/* Error status log */
> > +#define REG_IMGO_ERR_STAT 0x1360
> > +#define REG_RRZO_ERR_STAT 0x1364
> > +#define REG_AAO_ERR_STAT 0x1368
> > +#define REG_AFO_ERR_STAT 0x136c
> > +#define REG_LCSO_ERR_STAT 0x1370
> > +#define REG_UFEO_ERR_STAT 0x1374
> > +#define REG_PDO_ERR_STAT 0x1378
> > +#define REG_BPCI_ERR_STAT 0x137c
> > +#define REG_LSCI_ERR_STAT 0x1384
> > +#define REG_PDI_ERR_STAT 0x138c
> > +#define REG_LMVO_ERR_STAT 0x1390
> > +#define REG_FLKO_ERR_STAT 0x1394
> > +#define REG_PSO_ERR_STAT 0x13a0
> > +
> > +/* ISP command */
> > +#define REG_CQ_THR0_BASEADDR 0x0198
> > +#define REG_HW_FRAME_NUM 0x13b8
> > +
> > +/* META */
> > +#define REG_META0_VB2_INDEX 0x14dc
> > +#define REG_META1_VB2_INDEX 0x151c
>
> I don't believe these registers are really for VB2 indexes.
>
MTK P1 ISP HW supports frame header spare registers for each DMA, such
as CAM_DMA_FH_AAO_SPARE or CAM_DMA_FH_AFO_SPARE. We could save some
frame information in these ISP registers. In this case, we save META0
VB2 index into AAO FH spare register and META1 VB2 index into AFO FH
spare register. These implementation is designed for non-request 3A
DMAs. These VB2 indexes are sent in ISP_CMD_ENQUEUE_META command of
mtk_isp_enqueue function. So we just call CAM_DMA_FH_AAO_SPARE as
REG_META0_VB2_INDEX for easy understanding. Moreover, if we only need to
support request mode, we should remove this here.
cmd_params.cmd_id = ISP_CMD_ENQUEUE_META;
cmd_params.meta_frame.enabled_dma = dma_port;
cmd_params.meta_frame.vb_index = buffer->vbb.vb2_buf.index;
cmd_params.meta_frame.meta_addr.iova = buffer->daddr;
cmd_params.meta_frame.meta_addr.scp_addr = buffer->scp_addr;
> > +
> > +/* FBC */
> > +#define REG_AAO_FBC_STATUS 0x013c
> > +#define REG_AFO_FBC_STATUS 0x0134
> > +
> > +#endif /* _CAM_REGS_H */
> > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c
> > new file mode 100644
> > index 000000000000..c5a3babed69d
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c
> > @@ -0,0 +1,1087 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/cdev.h>
> > +#include <linux/compat.h>
> > +#include <linux/fs.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/kernel.h>
> > +#include <linux/ktime.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_data/mtk_scp.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/remoteproc.h>
> > +#include <linux/sched/clock.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +#include <linux/videodev2.h>
> > +#include <linux/vmalloc.h>
> > +#include <media/v4l2-event.h>
> > +
> > +#include "mtk_cam.h"
> > +#include "mtk_cam-regs.h"
> > +#include "mtk_cam-smem.h"
> > +
> > +static const struct of_device_id mtk_isp_of_ids[] = {
> > + {.compatible = "mediatek,mt8183-camisp",},
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_isp_of_ids);
>
> Please move below. Just above the platform_driver struct where it's used.
>
Fix in next patch.
> > +
> > +/* List of clocks required by isp cam */
> > +static const char * const mtk_isp_clks[] = {
> > + "camsys_cam_cgpdn", "camsys_camtg_cgpdn"
> > +};
>
> Please move inside mtk_isp_probe, as a static const local variable. That
> could also let you shorten the name, to clk_names for example.
>
Fix in next patch.
> > +
> > +static void isp_dump_dma_status(struct isp_device *isp_dev)
> > +{
> > + dev_err(isp_dev->dev,
> > + "IMGO:0x%x, RRZO:0x%x, AAO=0x%x, AFO=0x%x, LMVO=0x%x\n",
> > + readl(isp_dev->regs + REG_IMGO_ERR_STAT),
> > + readl(isp_dev->regs + REG_RRZO_ERR_STAT),
> > + readl(isp_dev->regs + REG_AAO_ERR_STAT),
> > + readl(isp_dev->regs + REG_AFO_ERR_STAT),
> > + readl(isp_dev->regs + REG_LMVO_ERR_STAT));
> > + dev_err(isp_dev->dev,
> > + "LCSO=0x%x, PSO=0x%x, FLKO=0x%x, BPCI:0x%x, LSCI=0x%x\n",
> > + readl(isp_dev->regs + REG_LCSO_ERR_STAT),
> > + readl(isp_dev->regs + REG_PSO_ERR_STAT),
> > + readl(isp_dev->regs + REG_FLKO_ERR_STAT),
> > + readl(isp_dev->regs + REG_BPCI_ERR_STAT),
> > + readl(isp_dev->regs + REG_LSCI_ERR_STAT));
> > +}
> > +
> > +static void mtk_cam_dev_event_frame_sync(struct mtk_cam_dev *cam_dev,
> > + __u32 frame_seq_no)
> > +{
> > + struct v4l2_event event;
> > +
> > + memset(&event, 0, sizeof(event));
> > + event.type = V4L2_EVENT_FRAME_SYNC;
> > + event.u.frame_sync.frame_sequence = frame_seq_no;
>
> nit: You can just initialize the structure in the declaration.
>
Fix in next patch.
void mtk_cam_dev_event_frame_sync(struct mtk_cam_dev *cam,
unsigned int frame_seq_no)
{
struct v4l2_event event = {
.type = V4L2_EVENT_FRAME_SYNC,
.u.frame_sync.frame_sequence = frame_seq_no,
};
v4l2_event_queue(cam->subdev.devnode, &event);
}
> > + v4l2_event_queue(cam_dev->subdev.devnode, &event);
> > +}
> > +
> > +static void mtk_cam_dev_job_finish(struct mtk_isp_p1_ctx *isp_ctx,
> > + unsigned int request_fd,
> > + unsigned int frame_seq_no,
> > + struct list_head *list_buf,
> > + enum vb2_buffer_state state)
> > +{
> > + struct isp_p1_device *p1_dev = p1_ctx_to_dev(isp_ctx);
> > + struct mtk_cam_dev *cam_dev = &p1_dev->cam_dev;
> > + struct mtk_cam_dev_buffer *buf, *b0;
> > + u64 timestamp;
>
> Too many spaces between u64 and timestamp.
>
Fix in next patch.
> > +
> > + if (!cam_dev->streaming)
> > + return;
> > +
> > + dev_dbg(&p1_dev->pdev->dev, "%s request fd:%d frame_seq:%d state:%d\n",
> > + __func__, request_fd, frame_seq_no, state);
> > +
> > + /*
> > + * Set the buffer's VB2 status so that the user can dequeue
> > + * the buffer.
> > + */
> > + timestamp = ktime_get_ns();
> > + list_for_each_entry_safe(buf, b0, list_buf, list) {
>
> nit: s/b0/buf_prev/
>
Fix in next patch.
> > + list_del(&buf->list);
> > + buf->vbb.vb2_buf.timestamp = timestamp;
> > + buf->vbb.sequence = frame_seq_no;
> > + if (buf->vbb.vb2_buf.state == VB2_BUF_STATE_ACTIVE)
>
> Any buffer that is not active shouldn't be on this list. If it is then it's
> a bug somewhere else in the driver. Could be possibly related to the request
> handling issues I pointed out in another comment.
>
Fix in next patch.
> > + vb2_buffer_done(&buf->vbb.vb2_buf, state);
> > + }
> > +}
> > +
> > +static void isp_deque_frame(struct isp_p1_device *p1_dev,
>
> dequeue
>
Fix in next patch.
> > + unsigned int node_id, int vb2_index,
> > + int frame_seq_no)
> > +{
> > + struct mtk_cam_dev *cam_dev = &p1_dev->cam_dev;
> > + struct device *dev = &p1_dev->pdev->dev;
> > + struct mtk_cam_video_device *node = &cam_dev->vdev_nodes[node_id];
> > + struct mtk_cam_dev_buffer *b, *b0;
> > + struct vb2_buffer *vb;
> > +
> > + if (!cam_dev->vdev_nodes[node_id].enabled || !cam_dev->streaming)
> > + return;
> > +
> > + spin_lock(&node->slock);
> > + b = list_first_entry(&node->pending_list,
> > + struct mtk_cam_dev_buffer,
> > + list);
> > + list_for_each_entry_safe(b, b0, &node->pending_list, list) {
> > + vb = &b->vbb.vb2_buf;
> > + if (!vb->vb2_queue->uses_requests &&
> > + vb->index == vb2_index &&
> > + vb->state == VB2_BUF_STATE_ACTIVE) {
> > + dev_dbg(dev, "%s:%d:%d", __func__, node_id, vb2_index);
> > + vb->timestamp = ktime_get_ns();
> > + b->vbb.sequence = frame_seq_no;
> > + vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> > + list_del(&b->list);
> > + break;
> > + }
> > + }
> > + spin_unlock(&node->slock);
> > +}
> > +
> > +static void isp_deque_request_frame(struct isp_p1_device *p1_dev,
>
> dequeue
>
Fix in next patch.
> > + int frame_seq_no)
> > +{
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + struct device *dev = &p1_dev->pdev->dev;
> > + struct mtk_isp_queue_job *framejob, *tmp;
> > + struct isp_queue *p1_enqueue_list = &isp_ctx->p1_enqueue_list;
> > +
> > + /* Match dequeue work and enqueue frame */
> > + spin_lock(&p1_enqueue_list->lock);
> > + list_for_each_entry_safe(framejob, tmp, &p1_enqueue_list->queue,
> > + list_entry) {
> > + dev_dbg(dev,
> > + "%s frame_seq_no:%d, target frame_seq_no:%d\n",
> > + __func__,
> > + framejob->frame_seq_no, frame_seq_no);
> > + /* Match by the en-queued request number */
> > + if (framejob->frame_seq_no == frame_seq_no) {
> > + /* Pass to user space */
> > + mtk_cam_dev_job_finish(isp_ctx,
> > + framejob->request_fd,
> > + framejob->frame_seq_no,
> > + &framejob->list_buf,
> > + VB2_BUF_STATE_DONE);
> > + atomic_dec(&p1_enqueue_list->queue_cnt);
> > + dev_dbg(dev,
> > + "frame_seq_no:%d is done, queue_cnt:%d\n",
> > + framejob->frame_seq_no,
> > + atomic_read(&p1_enqueue_list->queue_cnt));
> > +
> > + /* Remove only when frame ready */
> > + list_del(&framejob->list_entry);
> > + kfree(framejob);
> > + break;
> > + } else if (framejob->frame_seq_no < frame_seq_no) {
> > + /* Pass to user space for frame drop */
> > + mtk_cam_dev_job_finish(isp_ctx,
> > + framejob->request_fd,
> > + framejob->frame_seq_no,
> > + &framejob->list_buf,
> > + VB2_BUF_STATE_ERROR);
> > + atomic_dec(&p1_enqueue_list->queue_cnt);
> > + dev_warn(dev,
> > + "frame_seq_no:%d drop, queue_cnt:%d\n",
> > + framejob->frame_seq_no,
> > + atomic_read(&p1_enqueue_list->queue_cnt));
> > +
> > + /* Remove only drop frame */
> > + list_del(&framejob->list_entry);
> > + kfree(framejob);
> > + } else {
> > + break;
> > + }
> > + }
> > + spin_unlock(&p1_enqueue_list->lock);
> > +}
> > +
> > +static int isp_deque_work(void *data)
>
> dequeue
>
Fix in next patch.
> > +{
> > + struct isp_p1_device *p1_dev = (struct isp_p1_device *)data;
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + struct mtk_cam_dev_stat_event_data event_data;
> > + atomic_t *irq_data_end = &isp_ctx->irq_data_end;
> > + atomic_t *irq_data_start = &isp_ctx->irq_data_start;
> > + unsigned long flags;
> > + int ret, i;
> > +
> > + while (1) {
> > + ret = wait_event_interruptible(isp_ctx->isp_deque_thread.wq,
> > + (atomic_read(irq_data_end) !=
> > + atomic_read(irq_data_start)) ||
> > + kthread_should_stop());
> > +
> > + if (kthread_should_stop())
> > + break;
> > +
> > + spin_lock_irqsave(&isp_ctx->irq_dequeue_lock, flags);
> > + i = atomic_read(&isp_ctx->irq_data_start);
> > + memcpy(&event_data, &isp_ctx->irq_event_datas[i],
> > + sizeof(event_data));
> > + atomic_set(&isp_ctx->irq_data_start, ++i & 0x3);
> > + spin_unlock_irqrestore(&isp_ctx->irq_dequeue_lock, flags);
> > +
> > + if (event_data.irq_status_mask & HW_PASS1_DON_ST &&
> > + event_data.dma_status_mask & AAO_DONE_ST) {
> > + isp_deque_frame(p1_dev,
> > + MTK_CAM_P1_META_OUT_0,
> > + event_data.meta0_vb2_index,
> > + event_data.frame_seq_no);
> > + }
> > + if (event_data.dma_status_mask & AFO_DONE_ST) {
> > + isp_deque_frame(p1_dev,
> > + MTK_CAM_P1_META_OUT_1,
> > + event_data.meta1_vb2_index,
> > + event_data.frame_seq_no);
> > + }
> > + if (event_data.irq_status_mask & SW_PASS1_DON_ST) {
> > + isp_deque_frame(p1_dev,
> > + MTK_CAM_P1_META_OUT_0,
> > + event_data.meta0_vb2_index,
> > + event_data.frame_seq_no);
> > + isp_deque_frame(p1_dev,
> > + MTK_CAM_P1_META_OUT_1,
> > + event_data.meta1_vb2_index,
> > + event_data.frame_seq_no);
> > + isp_deque_request_frame(p1_dev,
> > + event_data.frame_seq_no);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int irq_handle_sof(struct isp_device *isp_dev,
> > + dma_addr_t base_addr,
> > + unsigned int frame_num)
> > +{
> > + unsigned int addr_offset;
> > + struct isp_p1_device *p1_dev = get_p1_device(isp_dev->dev);
> > + int cq_num = atomic_read(&p1_dev->isp_ctx.composed_frame_id);
> > +
> > + isp_dev->sof_count += 1;
> > +
> > + if (cq_num <= frame_num) {
> > + dev_dbg(isp_dev->dev,
> > + "SOF_INT_ST, wait next, cq_num:%d, frame_num:%d",
> > + cq_num, frame_num);
> > + atomic_set(&p1_dev->isp_ctx.composing_frame, 0);
> > + return cq_num;
> > + }
> > + atomic_set(&p1_dev->isp_ctx.composing_frame, cq_num - frame_num);
> > +
> > + addr_offset = CQ_ADDRESS_OFFSET * (frame_num % CQ_BUFFER_COUNT);
> > + writel(base_addr + addr_offset, isp_dev->regs + REG_CQ_THR0_BASEADDR);
> > + dev_dbg(isp_dev->dev,
> > + "SOF_INT_ST, update next, cq_num:%d, frame_num:%d cq_addr:0x%x",
> > + cq_num, frame_num, addr_offset);
> > +
> > + return cq_num;
> > +}
> > +
> > +static void irq_handle_notify_event(struct isp_device *isp_dev,
> > + unsigned int irq_status,
> > + unsigned int dma_status,
> > + bool sof_only)
> > +{
> > + struct isp_p1_device *p1_dev = get_p1_device(isp_dev->dev);
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + struct device *dev = isp_dev->dev;
> > + unsigned long flags;
> > + int i;
> > +
> > + if (irq_status & VS_INT_ST) {
> > + /* Notify specific HW events to user space */
> > + mtk_cam_dev_event_frame_sync(&p1_dev->cam_dev,
> > + isp_dev->current_frame);
>
> Shouldn't we call this at SOF_INT_ST and not VS? At least according to the
> definition of the V4L2_EVENT_FRAME_SYNC event at
> https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-dqevent.html
>
Fix in next patch.
We will change to use SOF_INT_ST to avoid misunderstanding.
> > + dev_dbg(dev,
> > + "frame sync is sent:%d:%d\n",
> > + isp_dev->sof_count,
> > + isp_dev->current_frame);
> > + if (sof_only)
> > + return;
>
> If this function can be called only to perform this block, perhaps it should
> be split into two functions?
>
> Also, what happens if we get sof_only, but we don't get VS_INT_ST set in
> irq_status? Is it expected that in such case the other part of the function
> is executed?
>
Ok, we will call mtk_cam_dev_event_frame_sync function when receiving
SOF_INT_ST ISR event in the caller and remove this block.
> > + }
> > +
> > + if (irq_status & SW_PASS1_DON_ST) {
> > + /* Notify TX thread to send if TX frame is blocked */
> > + wake_up_interruptible(&isp_ctx->composer_tx_thread.wq);
> > + }
> > +
> > + spin_lock_irqsave(&isp_ctx->irq_dequeue_lock, flags);
> > + i = atomic_read(&isp_ctx->irq_data_end);
> > + isp_ctx->irq_event_datas[i].frame_seq_no = isp_dev->current_frame;
> > + isp_ctx->irq_event_datas[i].meta0_vb2_index = isp_dev->meta0_vb2_index;
> > + isp_ctx->irq_event_datas[i].meta1_vb2_index = isp_dev->meta1_vb2_index;
> > + isp_ctx->irq_event_datas[i].irq_status_mask =
> > + (irq_status & INT_ST_MASK_CAM);
> > + isp_ctx->irq_event_datas[i].dma_status_mask =
> > + (dma_status & DMA_ST_MASK_CAM);
> > + atomic_set(&isp_ctx->irq_data_end, ++i & 0x3);
> > + spin_unlock_irqrestore(&isp_ctx->irq_dequeue_lock, flags);
> > +
> > + wake_up_interruptible(&isp_ctx->isp_deque_thread.wq);
>
> I can see that all isp_deque_work() does is returning buffers to vb2. I
> don't think we need this intricate system to do that, as we could just do
> it inside the interrupt handler, in isp_irq_cam() directly.
>
Ok, we will move all dequeue function in the ISR function and remove
this dequeue thread and related codes.
> > +
> > + dev_dbg(dev,
> > + "%s IRQ:0x%x DMA:0x%x seq:%d idx0:%d idx1:%d\n",
> > + __func__,
> > + (irq_status & INT_ST_MASK_CAM),
> > + (dma_status & DMA_ST_MASK_CAM),
> > + isp_dev->current_frame,
> > + isp_dev->meta0_vb2_index,
> > + isp_dev->meta1_vb2_index);
> > +}
> > +
> > +irqreturn_t isp_irq_cam(int irq, void *data)
> > +{
> > + struct isp_device *isp_dev = (struct isp_device *)data;
> > + struct isp_p1_device *p1_dev = get_p1_device(isp_dev->dev);
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + struct device *dev = isp_dev->dev;
> > + unsigned int cam_idx, cq_num, hw_frame_num;
> > + unsigned int meta0_vb2_index, meta1_vb2_index;
> > + unsigned int irq_status, err_status, dma_status;
> > + unsigned int aao_fbc, afo_fbc;
> > + unsigned long flags;
> > +
> > + /* Check the streaming is off or not */
> > + if (!p1_dev->cam_dev.streaming)
> > + return IRQ_HANDLED;
>
> This shouldn't be needed. The driver needs to unmask the interrupts in
> hardware registers when it starts streaming and mask them when it stops.
> Note that I mean the P1 hardware registers, not disable_irq(), which
> shouldn't be used.
>
Fix in next patch.
> > +
> > + cam_idx = isp_dev->isp_hw_module - ISP_CAM_A_IDX;
> > + cq_num = 0;
> > +
> > + spin_lock_irqsave(&isp_dev->spinlock_irq, flags);
> > + irq_status = readl(isp_dev->regs + REG_CTL_RAW_INT_STAT);
> > + dma_status = readl(isp_dev->regs + REG_CTL_RAW_INT2_STAT);
> > + hw_frame_num = readl(isp_dev->regs + REG_HW_FRAME_NUM);
> > + meta0_vb2_index = readl(isp_dev->regs + REG_META0_VB2_INDEX);
> > + meta1_vb2_index = readl(isp_dev->regs + REG_META1_VB2_INDEX);
>
> Hmm, reading vb2 buffer index from hardware registers? Was this hardware
> designed exclusively for V4L2? ;)
>
> Jokes aside, how does the hardware know V4L2 buffer indexes?
>
This is explained in the above.
> > + aao_fbc = readl(isp_dev->regs + REG_AAO_FBC_STATUS);
> > + afo_fbc = readl(isp_dev->regs + REG_AFO_FBC_STATUS);
> > + spin_unlock_irqrestore(&isp_dev->spinlock_irq, flags);
> > +
> > + /* Ignore unnecessary IRQ */
> > + if (!irq_status && (!(dma_status & DMA_ST_MASK_CAM)))
> > + return IRQ_HANDLED;
>
> Unnecessary IRQs should be masked in the hardware IRQ mask registers. If we
> get an interrupt without any unmasked hardware IRQs active in the status,
> that's an error somewhere and we should return IRQ_NONE.
>
Ok, we will check the IRQ EN register firstly and check any unmasked
IRQs for IRQ_NONE case.
> > +
> > + err_status = irq_status & INT_ST_MASK_CAM_ERR;
> > +
> > + /* Sof, done order check */
> > + if ((irq_status & SOF_INT_ST) && (irq_status & HW_PASS1_DON_ST)) {
> > + dev_dbg(dev, "sof_done block cnt:%d\n", isp_dev->sof_count);
> > +
> > + /* Notify IRQ event and enqueue frame */
> > + irq_handle_notify_event(isp_dev, irq_status, dma_status, 0);
> > + isp_dev->current_frame = hw_frame_num;
>
> What exactly is hw_frame_num? Shouldn't we assign it before notifying the
> event?
>
This is a another spare register for frame sequence number usage.
It comes from struct p1_frame_param:frame_seq_no which is sent by
SCP_ISP_FRAME IPI command. We will rename this to dequeue_frame_seq_no.
Is it a better understanding?
Below is our frame request handling in current design.
1. Buffer preparation
- Combined image buffers (IMGO/RRZO) + meta input buffer (Tuining) +
other meta histogram buffers (LCSO/LMVO) into one request.
- Accumulated one unique frame sequence number to each request and send
this request to the SCP composer to compose CQ (Command queue) buffer
via SCP_ISP_FRAME IPI command.
- CQ buffer is frame registers set. If ISP registers should be updated
per frame, these registers are configured in the CQ buffer, such as
frame sequence number, DMA addresses and tuning ISP registers.
- One frame request will be composed into one CQ buffer.Once CQ buffer
is composed done and kernel driver will receive ISP_CMD_FRAME_ACK with
its corresponding frame sequence number. Based on this, kernel driver
knows which request is ready to be en-queued and save this with
p1_dev->isp_ctx.composed_frame_id.
- The maximum number of CQ buffers in SCP is 3.
2. Buffer en-queue flow
- In order to configure correct CQ buffer setting before next SQF event,
it is depended on by MTK ISP P1 HW CQ mechanism.
- The basic concept of CQ mechanism is loaded ISP CQ buffer settings
when HW_PASS1_DON_ST is received which means DMA output is done.
- Btw, the pre-condition of this, need to tell ISP HW which CQ buffer
address is used. Otherwise, it will loaded one dummy CQ buffer to
bypass.
- So we will check available CQ buffers by comparing composed frame
sequence number & dequeued frame sequence from ISP HW in SOF event.
- If there are available CQ buffers, update the CQ base address to the
next CQ buffer address based on current de-enqueue frame sequence
number. So MTK ISP P1 HW will load this CQ buffer into HW when
HW_PASS1_DON_ST is triggered which is before the next SOF.
- So in next SOF event, ISP HW starts to output DMA buffers with this
request until request is done.
- But, for the first request, it is loaded into HW manually when
streaming is on for better performance.
3. Buffer de-queue flow
- We will use frame sequence number to decide which request is ready to
de-queue.
- We will save some important register setting from ISP HW when SOF is
received. This is because the ISP HW starts to output the data with the
corresponding settings, especially frame sequence number setting.
- When receiving SW_PASS1_DON_ST IRQ event, it means the DMA output is
done. So we could call isp_deque_request_frame with frame sequence
number to de-queue frame to VB2
- For AAO/AFO buffers, it has similar design concept. Sometimes, if only
AAO/AFO non-request buffers are en-queued without request buffers at the
same time, there will be no SW P1 done event for AAO/AFO DMA done.
Needs to depend on other IRQ events, such as AAO/AFO_DONE_EVENT.
- Due to CQ buffer number limitation, if we receive SW_PASS1_DONT_ST,
we may try to send another request to SCP for composing.
Hopefully, my explanation is helpful for better understanding our
implementation. If you still have any questions, please let me know.
> > + isp_dev->meta0_vb2_index = meta0_vb2_index;
> > + isp_dev->meta1_vb2_index = meta1_vb2_index;
> > + } else {
> > + if (irq_status & SOF_INT_ST) {
> > + isp_dev->current_frame = hw_frame_num;
> > + isp_dev->meta0_vb2_index = meta0_vb2_index;
> > + isp_dev->meta1_vb2_index = meta1_vb2_index;
> > + }
> > + irq_handle_notify_event(isp_dev, irq_status, dma_status, 1);
> > + }
>
> The if and else blocks do almost the same things just in different order. Is
> it really expected?
>
If we receive HW_PASS1_DON_ST & SOF_INT_ST IRQ events at the same time,
the correct sequence should be handle HW_PASS1_DON_ST firstly to check
any de-queued frame and update the next frame setting later.
Normally, this is a corner case or system performance issue.
Btw, we will revise the above implementation as below.
if (irq_status & SOF_INT_ST)
mtk_cam_dev_event_frame_sync(&p1_dev->cam_dev,
dequeue_frame_seq_no);
/* Sof, done order check */
if ((irq_status & SOF_INT_ST) && (irq_status & HW_PASS1_DON_ST))
dev_warn(dev, "sof_done block cnt:%d\n", p1_dev->sof_count);
/* Notify IRQ event and de-enqueue frame */
irq_handle_notify_event(p1_dev, irq_status, dma_status);
/* Update frame settings & CQ address for frame en-queue */
enqueue_frame_seq_no = 0;
if (irq_status & SOF_INT_ST)
enqueue_frame_seq_no = irq_handle_sof(p1_dev,
dequeue_frame_seq_no,
meta0_vb2_index,
meta1_vb2_index);
> > +
> > + if (irq_status & SOF_INT_ST)
> > + cq_num = irq_handle_sof(isp_dev, isp_ctx->scp_mem_iova,
> > + hw_frame_num);
> > +
> > + /* Check ISP error status */
> > + if (err_status) {
> > + dev_err(dev,
> > + "raw_int_err:0x%x/0x%x\n",
> > + irq_status, err_status);
> > + /* Show DMA errors in detail */
> > + if (err_status & DMA_ERR_ST)
> > + isp_dump_dma_status(isp_dev);
> > + }
> > +
> > + if (irq_status & INT_ST_LOG_MASK_CAM)
> > + dev_dbg(dev, IRQ_STAT_STR,
>
> Please just put that string here, otherwise the reader would have no idea
> what message is being printed here.
>
Fix in next patch.
> > + 'A' + cam_idx,
> > + isp_dev->sof_count,
> > + irq_status,
> > + dma_status,
> > + hw_frame_num,
> > + cq_num,
> > + aao_fbc,
> > + afo_fbc);
>
> nit: No need to put each argument in its own line.
>
Fix in next patch.
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int isp_setup_scp_rproc(struct isp_p1_device *p1_dev)
> > +{
> > + phandle rproc_phandle;
> > + struct device *dev = &p1_dev->pdev->dev;
> > + int ret;
> > +
> > + p1_dev->scp_pdev = scp_get_pdev(p1_dev->pdev);
> > + if (!p1_dev->scp_pdev) {
> > + dev_err(dev, "Failed to get scp device\n");
> > + return -ENODEV;
> > + }
> > +
> > + ret = of_property_read_u32(dev->of_node, "mediatek,scp",
> > + &rproc_phandle);
> > + if (ret) {
> > + dev_err(dev, "fail to get rproc_phandle:%d\n", ret);
> > + return -EINVAL;
> > + }
> > +
> > + p1_dev->rproc_handle = rproc_get_by_phandle(rproc_phandle);
> > + dev_dbg(dev, "p1 rproc_phandle: 0x%pK\n\n", p1_dev->rproc_handle);
> > + if (!p1_dev->rproc_handle) {
> > + dev_err(dev, "fail to get rproc_handle\n");
> > + return -EINVAL;
> > + }
>
> This look-up should be done once in probe(). Only the rproc_boot() should
> happen dynamically.
>
Fix in next patch.
> > +
> > + ret = rproc_boot(p1_dev->rproc_handle);
> > + if (ret) {
> > + /*
> > + * Return 0 if downloading firmware successfully,
> > + * otherwise it is failed
> > + */
> > + return -ENODEV;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int isp_init_context(struct isp_p1_device *p1_dev)
> > +{
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + struct device *dev = &p1_dev->pdev->dev;
> > + unsigned int i;
> > +
> > + dev_dbg(dev, "init irq work thread\n");
> > + if (!isp_ctx->isp_deque_thread.thread) {
> > + init_waitqueue_head(&isp_ctx->isp_deque_thread.wq);
> > + isp_ctx->isp_deque_thread.thread =
> > + kthread_run(isp_deque_work, (void *)p1_dev,
> > + "isp_deque_work");
> > + if (IS_ERR(isp_ctx->isp_deque_thread.thread)) {
> > + dev_err(dev, "unable to alloc kthread\n");
> > + isp_ctx->isp_deque_thread.thread = NULL;
> > + return -ENOMEM;
> > + }
> > + }
> > + spin_lock_init(&isp_ctx->irq_dequeue_lock);
> > + mutex_init(&isp_ctx->lock);
> > +
> > + INIT_LIST_HEAD(&isp_ctx->p1_enqueue_list.queue);
> > + atomic_set(&isp_ctx->p1_enqueue_list.queue_cnt, 0);
> > +
> > + for (i = 0; i < ISP_DEV_NODE_NUM; i++)
> > + spin_lock_init(&p1_dev->isp_devs[i].spinlock_irq);
> > +
> > + spin_lock_init(&isp_ctx->p1_enqueue_list.lock);
> > + spin_lock_init(&isp_ctx->composer_txlist.lock);
> > +
> > + atomic_set(&isp_ctx->irq_data_end, 0);
> > + atomic_set(&isp_ctx->irq_data_start, 0);
> > +
> > + return 0;
>
> Everything here looks like something that should be done once in probe. I
> also don't see a point of having a separate mtk_isp_p1_ctx struct for the
> data above. It could be just located in p1_dev, at least for now.
>
> If we end up implementing support for multiple contexts, we could isolate
> per-context data then, once we know what's really per-context. For now,
> let's keep it simple.
>
Ok, we will remove isp_ctx structure and move some fields into p1_dev or
cam_dev,
> > +}
> > +
> > +static int isp_uninit_context(struct device *dev)
> > +{
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + struct mtk_isp_queue_job *framejob, *tmp_framejob;
> > +
> > + spin_lock_irq(&isp_ctx->p1_enqueue_list.lock);
> > + list_for_each_entry_safe(framejob, tmp_framejob,
> > + &isp_ctx->p1_enqueue_list.queue, list_entry) {
> > + list_del(&framejob->list_entry);
> > + kfree(framejob);
> > + }
> > + spin_unlock_irq(&isp_ctx->p1_enqueue_list.lock);
> > +
> > + if (isp_ctx->isp_deque_thread.thread) {
> > + kthread_stop(isp_ctx->isp_deque_thread.thread);
> > + wake_up_interruptible(&isp_ctx->isp_deque_thread.wq);
> > + isp_ctx->isp_deque_thread.thread = NULL;
> > + }
> > +
> > + mutex_destroy(&isp_ctx->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static unsigned int get_enabled_dma_ports(struct mtk_cam_dev *cam_dev)
> > +{
> > + unsigned int enabled_dma_ports, i;
> > +
> > + /* Get the enabled meta DMA ports */
> > + enabled_dma_ports = 0;
> > +
> > + for (i = 0; i < MTK_CAM_P1_TOTAL_NODES; i++)
> > + if (cam_dev->vdev_nodes[i].enabled)
> > + enabled_dma_ports |=
> > + cam_dev->vdev_nodes[i].desc.dma_port;
> > +
> > + dev_dbg(&cam_dev->pdev->dev, "%s :0x%x", __func__, enabled_dma_ports);
> > +
> > + return enabled_dma_ports;
> > +}
> > +
> > +/* Utility functions */
> > +static unsigned int get_sensor_pixel_id(unsigned int fmt)
> > +{
> > + switch (fmt) {
> > + case MEDIA_BUS_FMT_SBGGR8_1X8:
> > + case MEDIA_BUS_FMT_SBGGR10_1X10:
> > + case MEDIA_BUS_FMT_SBGGR12_1X12:
> > + case MEDIA_BUS_FMT_SBGGR14_1X14:
> > + return RAW_PXL_ID_B;
> > + case MEDIA_BUS_FMT_SGBRG8_1X8:
> > + case MEDIA_BUS_FMT_SGBRG10_1X10:
> > + case MEDIA_BUS_FMT_SGBRG12_1X12:
> > + case MEDIA_BUS_FMT_SGBRG14_1X14:
> > + return RAW_PXL_ID_GB;
> > + case MEDIA_BUS_FMT_SGRBG8_1X8:
> > + case MEDIA_BUS_FMT_SGRBG10_1X10:
> > + case MEDIA_BUS_FMT_SGRBG12_1X12:
> > + case MEDIA_BUS_FMT_SGRBG14_1X14:
> > + return RAW_PXL_ID_GR;
> > + case MEDIA_BUS_FMT_SRGGB8_1X8:
> > + case MEDIA_BUS_FMT_SRGGB10_1X10:
> > + case MEDIA_BUS_FMT_SRGGB12_1X12:
> > + case MEDIA_BUS_FMT_SRGGB14_1X14:
> > + return RAW_PXL_ID_R;
> > + default:
>
> Could we fail here instead?
>
Ok, we will return invalid value here, like MTK_CAM_RAW_PXL_ID_UNKNOWN.
We will also check this error in the caller.
> > + return RAW_PXL_ID_B;
> > + }
> > +}
> > +
> > +static unsigned int get_sensor_fmt(unsigned int fmt)
> > +{
> > + switch (fmt) {
> > + case MEDIA_BUS_FMT_SBGGR8_1X8:
> > + case MEDIA_BUS_FMT_SGBRG8_1X8:
> > + case MEDIA_BUS_FMT_SGRBG8_1X8:
> > + case MEDIA_BUS_FMT_SRGGB8_1X8:
> > + return IMG_FMT_BAYER8;
> > + case MEDIA_BUS_FMT_SBGGR10_1X10:
> > + case MEDIA_BUS_FMT_SGBRG10_1X10:
> > + case MEDIA_BUS_FMT_SGRBG10_1X10:
> > + case MEDIA_BUS_FMT_SRGGB10_1X10:
> > + return IMG_FMT_BAYER10;
> > + case MEDIA_BUS_FMT_SBGGR12_1X12:
> > + case MEDIA_BUS_FMT_SGBRG12_1X12:
> > + case MEDIA_BUS_FMT_SGRBG12_1X12:
> > + case MEDIA_BUS_FMT_SRGGB12_1X12:
> > + return IMG_FMT_BAYER12;
> > + case MEDIA_BUS_FMT_SBGGR14_1X14:
> > + case MEDIA_BUS_FMT_SGBRG14_1X14:
> > + case MEDIA_BUS_FMT_SGRBG14_1X14:
> > + case MEDIA_BUS_FMT_SRGGB14_1X14:
> > + return IMG_FMT_BAYER14;
> > + default:
> > + return IMG_FMT_UNKNOWN;
> > + }
> > +}
> > +
> > +static unsigned int get_img_fmt(unsigned int fourcc)
> > +{
> > + switch (fourcc) {
> > + case V4L2_PIX_FMT_MTISP_B8:
> > + return IMG_FMT_BAYER8;
> > + case V4L2_PIX_FMT_MTISP_F8:
> > + return IMG_FMT_FG_BAYER8;
> > + case V4L2_PIX_FMT_MTISP_B10:
> > + return IMG_FMT_BAYER10;
> > + case V4L2_PIX_FMT_MTISP_F10:
> > + return IMG_FMT_FG_BAYER10;
> > + case V4L2_PIX_FMT_MTISP_B12:
> > + return IMG_FMT_BAYER12;
> > + case V4L2_PIX_FMT_MTISP_F12:
> > + return IMG_FMT_FG_BAYER12;
> > + case V4L2_PIX_FMT_MTISP_B14:
> > + return IMG_FMT_BAYER14;
> > + case V4L2_PIX_FMT_MTISP_F14:
> > + return IMG_FMT_FG_BAYER14;
> > + default:
> > + return IMG_FMT_UNKNOWN;
> > + }
> > +}
> > +
> > +static unsigned int get_pixel_byte(unsigned int fourcc)
> > +{
> > + switch (fourcc) {
> > + case V4L2_PIX_FMT_MTISP_B8:
> > + case V4L2_PIX_FMT_MTISP_F8:
> > + return 8;
> > + case V4L2_PIX_FMT_MTISP_B10:
> > + case V4L2_PIX_FMT_MTISP_F10:
> > + return 10;
> > + case V4L2_PIX_FMT_MTISP_B12:
> > + case V4L2_PIX_FMT_MTISP_F12:
> > + return 12;
> > + case V4L2_PIX_FMT_MTISP_B14:
> > + case V4L2_PIX_FMT_MTISP_F14:
> > + return 14;
> > + default:
>
> Could we fail here instead, so that we don't mask some potential errors?
>
Ok, we will return MTK_CAM_IMG_FMT_UNKNOWN and check this error in the
caller.
> > + return 10;
> > + }
> > +}
> > +
> > +static void config_img_fmt(struct device *dev, struct p1_img_output *out_fmt,
> > + const struct v4l2_format *in_fmt,
> > + const struct v4l2_subdev_format *sd_format)
> > +{
> > + out_fmt->img_fmt = get_img_fmt(in_fmt->fmt.pix_mp.pixelformat);
> > + out_fmt->pixel_byte = get_pixel_byte(in_fmt->fmt.pix_mp.pixelformat);
> > + out_fmt->size.w = in_fmt->fmt.pix_mp.width;
> > + out_fmt->size.h = in_fmt->fmt.pix_mp.height;
> > +
> > + out_fmt->size.stride = in_fmt->fmt.pix_mp.plane_fmt[0].bytesperline;
> > + out_fmt->size.xsize = in_fmt->fmt.pix_mp.plane_fmt[0].bytesperline;
>
> Please group operations on the same field together, i.e. remove the blank
> line above size.stride and add one blank line above size.w.
>
Fix in next patch.
> > +
> > + out_fmt->crop.left = 0x0;
> > + out_fmt->crop.top = 0x0;
> > +
>
> Remove the blank line.
>
Fix in next patch.
> > + out_fmt->crop.width = sd_format->format.width;
> > + out_fmt->crop.height = sd_format->format.height;
> > +
> > + WARN_ONCE(in_fmt->fmt.pix_mp.width > out_fmt->crop.width ||
> > + in_fmt->fmt.pix_mp.height > out_fmt->crop.height,
> > + "img out:%d:%d in:%d:%d",
> > + in_fmt->fmt.pix_mp.width, in_fmt->fmt.pix_mp.height,
> > + out_fmt->crop.width, out_fmt->crop.height);
>
> We should check this earlier and fail the streaming start if there is a
> mismatch between sensor and video node configuration.
>
Fix in next patch.
> > +
> > + dev_dbg(dev, "pixel_byte:%d img_fmt:0x%x\n",
> > + out_fmt->pixel_byte,
> > + out_fmt->img_fmt);
> > + dev_dbg(dev,
> > + "param:size=%0dx%0d, stride:%d, xsize:%d, crop=%0dx%0d\n",
> > + out_fmt->size.w, out_fmt->size.h,
> > + out_fmt->size.stride, out_fmt->size.xsize,
> > + out_fmt->crop.width, out_fmt->crop.height);
> > +}
> > +
> > +/* ISP P1 interface functions */
> > +int mtk_isp_power_init(struct mtk_cam_dev *cam_dev)
> > +{
> > + struct device *dev = &cam_dev->pdev->dev;
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + int ret;
> > +
> > + ret = isp_setup_scp_rproc(p1_dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = isp_init_context(p1_dev);
> > + if (ret)
> > + return ret;
>
> The above function doesn't really seem to be related to power management.
> Should it be called from subdev stream on?
>
We will rename this function to mtk_isp_hw_init.
But, it will be called when the first video node is streamed on.
This is because we need to initialize the HW firstly for sub-device
stream-on performance. We need to send some IPI commands, such as
ISP_CMD_INIT & ISP_CMD_CONFIG_META & ISP_CMD_ENQUEUE_META in this
timing.
> > +
> > + ret = isp_composer_init(dev);
> > + if (ret)
> > + goto composer_err;
>
> This also doesn't seem to be related to power management.
>
Ok, we will rename to mtk_isp_hw_init to avoid misunderstanding.
> > +
> > + pm_runtime_get_sync(dev);
> > +
> > + /* ISP HW INIT */
> > + isp_ctx->isp_hw_module = ISP_CAM_B_IDX;
>
> There is some amount of code handling various values of isp_hw_module in
> this driver. If we're hardcoding ISP_CAM_B_IDX here, it's basically dead
> code that can't be tested. Please either add support for all the indexes in
> the driver or simplify all the code to just handle CAM_B.
>
Ok, we will simplify driver code to support single CAM only.
We will remove isp_hw_module usage in the source code.
> > + /* Use pure RAW as default HW path */
> > + isp_ctx->isp_raw_path = ISP_PURE_RAW_PATH;
> > + atomic_set(&p1_dev->cam_dev.streamed_node_count, 0);
> > +
> > + isp_composer_hw_init(dev);
> > + /* Check enabled DMAs which is configured by media setup */
> > + isp_composer_meta_config(dev, get_enabled_dma_ports(cam_dev));
>
> Hmm, this seems to be also configured by isp_compoer_hw_config(). Are both
> necessary?
>
Yes, it is necessary for non-request buffers design for Camera 3A video
nodes. For 3A video nodes, we just want to know which 3A video nodes are
enabled in ISP_CMD_CONFIG_META. In this stage, we may not know the image
format from user space. So we just pass the enabled DMA information from
kernel to SCP only. With 3A enabled DMA information, we could configure
related 3A registers in SCP.
> > +
> > + dev_dbg(dev, "%s done\n", __func__);
> > +
> > + return 0;
> > +
> > +composer_err:
> > + isp_uninit_context(dev);
> > +
> > + return ret;
> > +}
> > +
> > +int mtk_isp_power_release(struct device *dev)
> > +{
> > + isp_composer_hw_deinit(dev);
> > + isp_uninit_context(dev);
>
> These two don't seem to be related to power either.
>
> Instead, I don't see anything that could undo the rproc_boot() operation
> here.
>
We will rename this function to mtk_isp_hw_release.
We will also add rproc_shutdown function call here.
int mtk_isp_hw_release(struct mtk_cam_dev *cam)
{
struct device *dev = cam->dev;
struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(dev);
isp_composer_hw_deinit(p1_dev);
pm_runtime_put_sync_autosuspend(dev);
rproc_shutdown(p1_dev->rproc_handle);
dev_dbg(dev, "%s done\n", __func__);
return 0;
}
> > +
> > + dev_dbg(dev, "%s done\n", __func__);
> > +
> > + return 0;
> > +}
> > +
> > +int mtk_isp_config(struct device *dev)
> > +{
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + struct p1_config_param config_param;
> > + struct mtk_cam_dev *cam_dev = &p1_dev->cam_dev;
> > + struct v4l2_subdev_format sd_fmt;
> > + unsigned int enabled_dma_ports;
> > + struct v4l2_format *img_fmt;
> > + int ret;
> > +
> > + p1_dev->isp_devs[isp_ctx->isp_hw_module].current_frame = 0;
> > + p1_dev->isp_devs[isp_ctx->isp_hw_module].sof_count = 0;
> > +
> > + isp_ctx->frame_seq_no = 1;
> > + atomic_set(&isp_ctx->composed_frame_id, 0);
> > +
> > + /* Get the enabled DMA ports */
> > + enabled_dma_ports = get_enabled_dma_ports(cam_dev);
> > + dev_dbg(dev, "%s enable_dma_ports:0x%x", __func__, enabled_dma_ports);
> > +
> > + /* Sensor config */
> > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + ret = v4l2_subdev_call(cam_dev->sensor, pad, get_fmt, NULL, &sd_fmt);
> > +
>
> Unnecessary blank line.
>
Fix in next patch.
> > + if (ret) {
> > + dev_dbg(dev, "sensor g_fmt on failed:%d\n", ret);
> > + return -EPERM;
>
> return ret?
>
Fix in next patch.
> > + }
> > +
> > + dev_dbg(dev,
> > + "get_fmt ret=%d, w=%d, h=%d, code=0x%x, field=%d, color=%d\n",
> > + ret, sd_fmt.format.width, sd_fmt.format.height,
> > + sd_fmt.format.code, sd_fmt.format.field,
> > + sd_fmt.format.colorspace);
> > +
> > + config_param.cfg_in_param.continuous = 0x1;
> > + config_param.cfg_in_param.subsample = 0x0;
> > + /* Fix to one pixel mode in default */
> > + config_param.cfg_in_param.pixel_mode = 0x1;
> > + /* Support normal pattern in default */
> > + config_param.cfg_in_param.data_pattern = 0x0;
> > +
> > + config_param.cfg_in_param.crop.left = 0x0;
> > + config_param.cfg_in_param.crop.top = 0x0;
>
> Why hexadecimal numerals? Also, what's the meaning of these values? For
> anything boolean, you could just use true and false as a substitute of 0 and
> 1. For anything that has more values, please define the values using macros.
>
Fix in next patch.
1. Remove unnecessary hexadecimal number usage
2. Use boolean value if possible
3. Assign non-empty field values.
Below is the fixed version.
config_param.cfg_in_param.continuous = true;
/* Fix to one pixel mode in default */
config_param.cfg_in_param.pixel_mode = MTK_ISP_ONE_PIXEL_MODE;
config_param.cfg_in_param.crop.width = sd_fmt.format.width;
config_param.cfg_in_param.crop.height = sd_fmt.format.height;
config_param.cfg_in_param.raw_pixel_id =
get_sensor_pixel_id(sd_fmt.format.code);
config_param.cfg_in_param.img_fmt = get_sensor_fmt(sd_fmt.format.code);
if (config_param.cfg_in_param.img_fmt == MTK_CAM_IMG_FMT_UNKNOWN ||
config_param.cfg_in_param.raw_pixel_id ==
MTK_CAM_RAW_PXL_ID_UNKNOWN) {
dev_err(dev, "Unknown cfg img fmt or unknown raw pixel id\n");
return -EINVAL;
}
> > +
> > + config_param.cfg_in_param.raw_pixel_id =
> > + get_sensor_pixel_id(sd_fmt.format.code);
> > + config_param.cfg_in_param.img_fmt = get_sensor_fmt(sd_fmt.format.code);
> > + config_param.cfg_in_param.crop.width = sd_fmt.format.width;
> > + config_param.cfg_in_param.crop.height = sd_fmt.format.height;
>
> Move the other crop settings from above to here.
>
Ditto.
> > +
> > + config_param.cfg_main_param.bypass = 1;
> > + img_fmt = &cam_dev->vdev_nodes[MTK_CAM_P1_MAIN_STREAM_OUT].vdev_fmt;
> > + if ((enabled_dma_ports & R_IMGO) == R_IMGO) {
>
> No need for the == R_IMGO part.
>
Since R_IMGO is mandatory video node, we will remove this checking.
> > + config_param.cfg_main_param.bypass = 0;
> > + config_param.cfg_main_param.pure_raw = isp_ctx->isp_raw_path;
> > + config_param.cfg_main_param.pure_raw_pack = 1;
> > + config_img_fmt(dev, &config_param.cfg_main_param.output,
> > + img_fmt, &sd_fmt);
> > + }
> > +
> > + config_param.cfg_resize_param.bypass = 1;
> > + img_fmt = &cam_dev->vdev_nodes[MTK_CAM_P1_PACKED_BIN_OUT].vdev_fmt;
> > + if ((enabled_dma_ports & R_RRZO) == R_RRZO) {
>
> Ditto.
>
Fix in next patch as below:
if (enabled_dma_ports & R_RRZO) {
ret = config_img_fmt(cam,
&config_param.cfg_resize_param.output,
img_fmt, &sd_fmt);
if (ret)
return ret;
} else {
config_param.cfg_resize_param.bypass = true;
}
> > + config_param.cfg_resize_param.bypass = 0;
> > + config_img_fmt(dev, &config_param.cfg_resize_param.output,
> > + img_fmt, &sd_fmt);
> > + }
> > +
> > + /* Configure meta DMAs info. */
> > + config_param.cfg_meta_param.enabled_meta_dmas = enabled_dma_ports;
>
> Should image DMAs be masked out of this bitmap?
>
We will replace struct cfg_meta_param with enabled_dmas integer.
So we can pass all enabled DMA masks to SCP.
> > +
> > + isp_composer_hw_config(dev, &config_param);
> > +
> > + dev_dbg(dev, "%s done\n", __func__);
> > +
> > + return 0;
> > +}
> > +
> > +void mtk_isp_enqueue(struct device *dev, unsigned int dma_port,
> > + struct mtk_cam_dev_buffer *buffer)
> > +{
> > + struct mtk_isp_scp_p1_cmd frameparams;
> > +
> > + memset(&frameparams, 0, sizeof(frameparams));
> > + frameparams.cmd_id = ISP_CMD_ENQUEUE_META;
> > + frameparams.meta_frame.enabled_dma = dma_port;
> > + frameparams.meta_frame.vb_index = buffer->vbb.vb2_buf.index;
> > + frameparams.meta_frame.meta_addr.iova = buffer->daddr;
> > + frameparams.meta_frame.meta_addr.scp_addr = buffer->scp_addr;
> > +
> > + isp_composer_enqueue(dev, &frameparams, SCP_ISP_CMD);
> > +}
> > +
> > +void mtk_isp_req_flush_buffers(struct device *dev)
> > +{
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
> > + struct mtk_isp_queue_job *job, *j0;
> > + struct mtk_cam_dev_buffer *buf, *b0;
> > + struct isp_queue *p1_list = &p1_dev->isp_ctx.p1_enqueue_list;
> > +
> > + if (!atomic_read(&p1_list->queue_cnt))
> > + return;
>
> Do we need this explicit check? The code below wouldn't do anything if there
> isn't anything in the list. IMHO we could even remove queue_cnt completely.
>
Since we have redesign request en-queue mechanism, this function will be
removed.
> > +
> > + spin_lock(&p1_list->lock);
> > + list_for_each_entry_safe(job, j0, &p1_list->queue, list_entry) {
>
> nit: s/j0/job_prev/
>
Will apply this naming rule in next patch.
> > + list_for_each_entry_safe(buf, b0, &job->list_buf, list) {
>
> nit: s/b0/buf_pref/
>
Ditto.
> Also, we should be able to replace this with iterating over the generic list
> of request objects, rather than this internal list.
>
> > + list_del(&buf->list);
> > + if (buf->vbb.vb2_buf.state == VB2_BUF_STATE_ACTIVE)
>
> It shouldn't be possible to happen. If you see this check failing, that
> means a problem somewhere else in the driver.
>
Fix in next patch.
> > + vb2_buffer_done(&buf->vbb.vb2_buf,
> > + VB2_BUF_STATE_ERROR);
> > + }
> > + list_del(&job->list_entry);
> > + atomic_dec(&p1_list->queue_cnt);
> > + kfree(job);
> > + }
> > + spin_unlock(&p1_list->lock);
> > +}
> > +
> > +void mtk_isp_req_enqueue(struct device *dev, struct media_request *req)
> > +{
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
>
> Just pass p1_dev to this function instead of dev.
>
We got your point.
Below is new function declaration.
void mtk_isp_req_enqueue(struct mtk_cam_dev *cam,
struct mtk_cam_dev_request *req)
> > + struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > + struct p1_frame_param frameparams;
> > + struct mtk_isp_queue_job *framejob;
> > + struct media_request_object *obj, *obj_safe;
> > + struct vb2_buffer *vb;
> > + struct mtk_cam_dev_buffer *buf;
> > +
> > + framejob = kzalloc(sizeof(*framejob), GFP_ATOMIC);
>
> This allocation shouldn't be needed. The structure should be already a part
> of the mtk_cam_dev_request struct that wraps media_request. Actually. I'd
> even say that the contents of this struct should be just moved to that one
> to avoid overabstracting.
>
For this function, we will apply the new design from P2 driver's
comment. Here is the new implementation.
struct mtk_cam_dev_request {
struct media_request req;
struct mtk_p1_frame_param frame_params;
struct work_struct frame_work;
struct list_head list;
atomic_t buf_count;
};
void mtk_isp_req_enqueue(struct mtk_cam_dev *cam,
struct mtk_cam_dev_request *req)
{
struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(cam->dev);
int ret;
req->frame_params.frame_seq_no = ++p1_dev->enqueue_frame_seq_no;
INIT_WORK(&req->frame_work, isp_tx_frame_worker);
ret = queue_work(p1_dev->composer_wq, &req->frame_work);
if (!ret)
dev_dbg(cam->dev, "frame_no:%d queue_work failed\n",
req->frame_params.frame_seq_no, ret);
else
dev_dbg(cam->dev, "Enqueue fd:%s frame_seq_no:%d job cnt:%d\n",
req->req.debug_str, req->frame_params.frame_seq_no,
atomic_read(&cam->running_job_count));
}
> > + memset(framejob, 0, sizeof(*framejob));
>
> Putting the above comment aside, kzalloc() already zeroes the memory.
>
Ditto.
> > + memset(&frameparams, 0, sizeof(frameparams));
> > + INIT_LIST_HEAD(&framejob->list_buf);
> > +
> > + frameparams.frame_seq_no = isp_ctx->frame_seq_no++;
> > + frameparams.sof_idx =
> > + p1_dev->isp_devs[isp_ctx->isp_hw_module].sof_count;
>
> How is this synchronized with the sof_count increment in irq_handle_sof()?
>
The sof_idx field is only used for debugging purpose.
We will remove this.
> > + framejob->frame_seq_no = frameparams.frame_seq_no;
> > +
> > + list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
> > + vb = container_of(obj, struct vb2_buffer, req_obj);
>
> We should check that the object type before assuming it's a vb2_buffer by
> calling vb2_request_object_is_buffer().
>
Fix in next patch.
> > + buf = container_of(vb, struct mtk_cam_dev_buffer, vbb.vb2_buf);
> > + framejob->request_fd = buf->vbb.request_fd;
>
> We shouldn't use request_fd as the key here. We already have req here, which
> is the right key to use.
>
> That said, I can see framejob->request_fd only used for printing a debugging
> message in mtk_cam_dev_job_finish(). The request API core should already
> print something for us once a request is completed, so perhaps that isn't
> needed?
>
Fix in next patch.
> > + frameparams.dma_buffers[buf->node_id].iova = buf->daddr;
> > + frameparams.dma_buffers[buf->node_id].scp_addr = buf->scp_addr;
> > + list_add_tail(&buf->list, &framejob->list_buf);
>
> Why do we need this private list? We could just call exactly the same
> list_for_each() over the request objects.
>
This function is re-designed.
> > + }
> > +
> > + spin_lock(&isp_ctx->p1_enqueue_list.lock);
> > + list_add_tail(&framejob->list_entry, &isp_ctx->p1_enqueue_list.queue);
>
> We already have a list head in mtk_cam_dev_request.
>
This function is re-designed.
> > + atomic_inc(&isp_ctx->p1_enqueue_list.queue_cnt);
> > + spin_unlock(&isp_ctx->p1_enqueue_list.lock);
> > +
> > + isp_composer_enqueue(dev, &frameparams, SCP_ISP_FRAME);
> > + dev_dbg(dev, "request fd:%d frame_seq_no:%d is queued cnt:%d\n",
> > + framejob->request_fd,
> > + frameparams.frame_seq_no,
> > + atomic_read(&isp_ctx->p1_enqueue_list.queue_cnt));
> > +}
> > +
> > +static int enable_sys_clock(struct isp_p1_device *p1_dev)
> > +{
> > + struct device *dev = &p1_dev->pdev->dev;
> > + int ret;
> > +
> > + dev_info(dev, "- %s\n", __func__);
>
> dev_dbg()
>
Fix in next patch.
> > +
> > + ret = clk_bulk_prepare_enable(p1_dev->isp_ctx.num_clks,
> > + p1_dev->isp_ctx.clk_list);
> > + if (ret)
> > + goto clk_err;
> > +
> > + return 0;
> > +
> > +clk_err:
> > + dev_err(dev, "cannot pre-en isp_cam clock:%d\n", ret);
> > + clk_bulk_disable_unprepare(p1_dev->isp_ctx.num_clks,
> > + p1_dev->isp_ctx.clk_list);
>
> clk_bulk_prepare_enable() returns without any clocks enabled if it fails, so
> this would disable the clocks second time.
>
Fix in next patch.
> > + return ret;
> > +}
> > +
> > +static void disable_sys_clock(struct isp_p1_device *p1_dev)
> > +{
> > + dev_info(&p1_dev->pdev->dev, "- %s\n", __func__);
>
> dev_dbg()
>
Fix in next patch.
> > + clk_bulk_disable_unprepare(p1_dev->isp_ctx.num_clks,
> > + p1_dev->isp_ctx.clk_list);
> > +}
>
> There is no point in having wrapper functions to just call one function
> inside. Please just call clk_bulk_*() directly.
>
Fix in next patch.
> > +
> > +static int mtk_isp_suspend(struct device *dev)
> > +{
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
> > + int module = p1_dev->isp_ctx.isp_hw_module;
> > + struct isp_device *isp_dev = &p1_dev->isp_devs[module];
> > + unsigned int reg_val;
> > +
> > + dev_dbg(dev, "- %s\n", __func__);
> > +
>
> We need to check if the device isn't already runtime suspended. If it is, we
> don't have to do anything here and can just return.
>
>
Add pm_runtime_suspended(dev) to check.
> We also need to ensure that no new requests are queued to the hardware at
> this point. This could be done by replacing any of the kthreads with
> workqueues and making all of the workqueues freezable.
>
Yes, we will use workqueue to send frame request.
Here is the workqueue's definition.
p1_dev->composer_wq =
alloc_ordered_workqueue(dev_name(p1_dev->dev),
__WQ_LEGACY | WQ_MEM_RECLAIM |
WQ_FREEZABLE);
> > + isp_dev = &p1_dev->isp_devs[module];
> > + reg_val = readl(isp_dev->regs + REG_TG_VF_CON);
> > + if (reg_val & VFDATA_EN_BIT) {
> > + dev_dbg(dev, "Cam:%d suspend, disable VF\n", module);
> > + /* Disable view finder */
> > + writel((reg_val & (~VFDATA_EN_BIT)),
> > + isp_dev->regs + REG_TG_VF_CON);
> > + /*
> > + * After VF enable, the TG frame count will be reset to 0;
> > + */
> > + reg_val = readl(isp_dev->regs + REG_TG_SEN_MODE);
> > + writel((reg_val & (~CMOS_EN_BIT)),
> > + isp_dev->regs + + REG_TG_SEN_MODE);
> > + }
>
> Are you sure this is the right handling? We need to make sure the hardware
> finishes processing the current frame and stops.
>
We will revise this handling to make sure the ISP HW is idle before
suspend.
> > +
> > + disable_sys_clock(p1_dev);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_isp_resume(struct device *dev)
> > +{
> > + struct isp_p1_device *p1_dev = get_p1_device(dev);
> > + int module = p1_dev->isp_ctx.isp_hw_module;
> > + struct isp_device *isp_dev = &p1_dev->isp_devs[module];
> > + unsigned int reg_val;
> > +
> > + dev_dbg(dev, "- %s\n", __func__);
> > +
>
> We need to check runtime PM status here as well, because if the device was
> suspended, there is nothing to do here.
>
Ditto.
> > + enable_sys_clock(p1_dev);
> > +
> > + /* V4L2 stream-on phase & restore HW stream-on status */
> > + if (p1_dev->cam_dev.streaming) {
> > + dev_dbg(dev, "Cam:%d resume,enable VF\n", module);
> > + /* Enable CMOS */
> > + reg_val = readl(isp_dev->regs + REG_TG_SEN_MODE);
> > + writel((reg_val | CMOS_EN_BIT),
> > + isp_dev->regs + REG_TG_SEN_MODE);
> > + /* Enable VF */
> > + reg_val = readl(isp_dev->regs + REG_TG_VF_CON);
> > + writel((reg_val | VFDATA_EN_BIT),
> > + isp_dev->regs + REG_TG_VF_CON);
> > + }
>
> Does the hardware keep all the state, e.g. queued buffers, during suspend?
> Would the code above continue all the capture from the next buffer, as
> queued by the userspace before the suspend?
>
Yes, we will test it.
1. SCP buffers are kept by SCP processor
2. ISP registers are still kept even if ISP clock is disable.
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_isp_probe(struct platform_device *pdev)
> > +{
> > + struct isp_p1_device *p1_dev;
> > + struct mtk_isp_p1_ctx *isp_ctx;
> > + struct isp_device *isp_dev;
> > + struct device *dev = &pdev->dev;
> > + struct resource *res;
> > + int irq;
> > + int ret;
> > + unsigned int i;
> > +
> > + p1_dev = devm_kzalloc(dev, sizeof(*p1_dev), GFP_KERNEL);
> > + if (!p1_dev)
> > + return -ENOMEM;
> > +
> > + dev_set_drvdata(dev, p1_dev);
> > + isp_ctx = &p1_dev->isp_ctx;
> > + p1_dev->pdev = pdev;
>
> Perhaps you want to store &pdev->dev instead of pdev? I'm not sure a
> reference to platform_device is very useful later in the code.
>
Fix in next patch.
> > +
> > + for (i = ISP_CAMSYS_CONFIG_IDX; i < ISP_DEV_NODE_NUM; i++) {
>
> I think we want to start from 0 here?
>
Because of single CAM support, we will revise our DTS tree to support
single CAM only. So we could remove this loop and check the CAM-B HW
information here. Here is below new function.
static int mtk_isp_probe(struct platform_device *pdev)
{
/* List of clocks required by isp cam */
static const char * const clk_names[] = {
"camsys_cam_cgpdn", "camsys_camtg_cgpdn"
};
struct mtk_isp_p1_device *p1_dev;
struct device *dev = &pdev->dev;
struct resource *res;
int irq, ret, i;
p1_dev = devm_kzalloc(dev, sizeof(*p1_dev), GFP_KERNEL);
if (!p1_dev)
return -ENOMEM;
p1_dev->dev = dev;
dev_set_drvdata(dev, p1_dev);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
p1_dev->regs = devm_ioremap_resource(dev, res);
if (IS_ERR(p1_dev->regs)) {
dev_err(dev, "Failed platform resources map\n");
return PTR_ERR(p1_dev->regs);
}
dev_dbg(dev, "cam, map_addr=0x%pK\n", p1_dev->regs);
irq = platform_get_irq(pdev, 0);
if (!irq) {
dev_err(dev, "Missing IRQ resources data\n");
return -ENODEV;
}
ret = devm_request_irq(dev, irq, isp_irq_cam, IRQF_SHARED,
dev_name(dev), p1_dev);
if (ret) {
dev_err(dev, "req_irq fail, dev:%s irq=%d\n",
dev->of_node->name, irq);
return ret;
}
dev_dbg(dev, "Reg. irq=%d, isr:%s\n", irq, dev_driver_string(dev));
spin_lock_init(&p1_dev->spinlock_irq);
p1_dev->num_clks = ARRAY_SIZE(clk_names);
p1_dev->clks = devm_kcalloc(dev, p1_dev->num_clks,
sizeof(*p1_dev->clks), GFP_KERNEL);
if (!p1_dev->clks)
return -ENOMEM;
for (i = 0; i < p1_dev->num_clks; ++i)
p1_dev->clks[i].id = clk_names[i];
ret = devm_clk_bulk_get(dev, p1_dev->num_clks, p1_dev->clks);
if (ret) {
dev_err(dev, "cannot get isp cam clock:%d\n", ret);
return ret;
}
ret = isp_setup_scp_rproc(p1_dev, pdev);
if (ret)
return ret;
pm_runtime_enable(dev);
/* Initialize the v4l2 common part */
ret = mtk_cam_dev_init(pdev, &p1_dev->cam_dev);
if (ret)
return ret;
return 0;
}
> > + isp_dev = &p1_dev->isp_devs[i];
> > + isp_dev->isp_hw_module = i;
> > + isp_dev->dev = dev;
>
> p1_dev already includes a pointer to this.
>
Fix in next patch.
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > + isp_dev->regs = devm_ioremap_resource(dev, res);
> > +
> > + dev_dbg(dev, "cam%u, map_addr=0x%lx\n",
> > + i, (unsigned long)isp_dev->regs);
> > +
> > + if (!isp_dev->regs)
>
> devm_ioremap_resource() returns ERR_PTR() not NULL on error.
>
Fix in next patch.
> > + return PTR_ERR(isp_dev->regs);
> > +
> > + /* Support IRQ from ISP_CAM_A_IDX */
> > + if (i >= ISP_CAM_A_IDX) {
> > + /* Reg & interrupts index is shifted with 1 */
>
> The reader can already see that it's shifted from the code below. The
> comment should say _why_ it is shifted.
>
ok, we will remove this checking after supporting single CAM HW.
> > + irq = platform_get_irq(pdev, i - 1);
>
> The bindings have 3 IRQs, but we only seem to request 2 here. Is that
> expected?
>
ok, we will remove this checking after supporting single CAM HW.
> > + if (irq) {
>
> Please invert this if, so that it bails out on error. Also, this should
> check for negative errors codes, not 0.
>
Fix in next patch.
> > + ret = devm_request_irq(dev, irq,
> > + isp_irq_cam,
> > + IRQF_SHARED,
> > + dev_driver_string(dev),
>
> Use dev_name().
>
Fix in next patch.
> > + (void *)isp_dev);
>
> No need to cast to void *.
>
Fix in next patch.
> > + if (ret) {
> > + dev_err(dev,
> > + "req_irq fail, dev:%s irq=%d\n",
> > + dev->of_node->name,
> > + irq);
> > + return ret;
> > + }
> > + dev_dbg(dev, "Registered irq=%d, ISR:%s\n",
> > + irq, dev_driver_string(dev));
> > + }
> > + }
> > + spin_lock_init(&isp_dev->spinlock_irq);
> > + }
>
> We might want to move out the body of this loop to a separate function to
> keep this function shorter.
>
Since we have already remove this loop, maybe we don't need to move out
the body into one single function.
> > +
> > + p1_dev->isp_ctx.num_clks = ARRAY_SIZE(mtk_isp_clks);
> > + p1_dev->isp_ctx.clk_list =
>
> nit: "list" is the term commonly used for the list data structure. There is
> also a convention to call the length of array XXX num_XXX, so how about
> clks and num_clks?
>
Fix in next patch.
> > + devm_kcalloc(dev,
> > + p1_dev->isp_ctx.num_clks,
> > + sizeof(*p1_dev->isp_ctx.clk_list),
> > + GFP_KERNEL);
> > + if (!p1_dev->isp_ctx.clk_list)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < p1_dev->isp_ctx.num_clks; ++i)
> > + p1_dev->isp_ctx.clk_list->id = mtk_isp_clks[i];
>
> Shouldn't this be clk_list[i].id?
>
Fix in next patch.
> > +
> > + ret = devm_clk_bulk_get(dev,
> > + p1_dev->isp_ctx.num_clks,
> > + p1_dev->isp_ctx.clk_list);
> > + if (ret) {
> > + dev_err(dev, "cannot get isp cam clock:%d\n", ret);
> > + return ret;
> > + }
> > +
> > + /* Initialize reserved DMA memory */
> > + ret = mtk_cam_reserved_memory_init(p1_dev);
> > + if (ret) {
> > + dev_err(dev, "failed to configure DMA memory:%d\n", ret);
> > + goto err_init;
> > + }
> > +
> > + /* Initialize the v4l2 common part */
> > + ret = mtk_cam_dev_init(pdev, &p1_dev->cam_dev);
> > + if (ret)
> > + goto err_init;
> > +
> > + spin_lock_init(&isp_ctx->p1_enqueue_list.lock);
> > + pm_runtime_enable(dev);
> > +
> > + return 0;
> > +
> > +err_init:
> > + if (p1_dev->cam_dev.smem_dev)
> > + device_unregister(p1_dev->cam_dev.smem_dev);
> > +
> > + return ret;
> > +}
> > +
> > +static int mtk_isp_remove(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct isp_p1_device *p1_dev = dev_get_drvdata(dev);
> > +
> > + pm_runtime_disable(dev);
> > + mtk_cam_dev_release(pdev, &p1_dev->cam_dev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_isp_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mtk_isp_suspend, mtk_isp_resume)
> > + SET_RUNTIME_PM_OPS(mtk_isp_suspend, mtk_isp_resume, NULL)
>
> For V4L2 drivers system and runtime PM ops would normally be completely
> different. Runtime PM ops would be called when the hardware is idle already
> or is about to become active. System PM ops would be called at system power
> state change and the hardware might be both idle or active. Please also see
> my comments to mtk_isp_suspend() and mtk_isp_resume() above.
>
Here is the new implementation. It should be clear to show the
difference between system and runtime PM ops.
static const struct dev_pm_ops mtk_isp_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
pm_runtime_force_resume)
SET_RUNTIME_PM_OPS(mtk_isp_runtime_suspend, mtk_isp_runtime_resume,
NULL)
};
> > +};
> > +
> > +static struct platform_driver mtk_isp_driver = {
> > + .probe = mtk_isp_probe,
> > + .remove = mtk_isp_remove,
> > + .driver = {
> > + .name = "mtk-cam",
>
> Shouldn't this be something like mtk-cam-p1? Please make sure this
> reasonably consistent with other drivers.
>
Fix in next patch.
> > + .of_match_table = of_match_ptr(mtk_isp_of_ids),
> > + .pm = &mtk_isp_pm_ops,
> > + }
> > +};
> > +
> > +module_platform_driver(mtk_isp_driver);
> > +
> > +MODULE_DESCRIPTION("Camera ISP driver");
>
> Mediatek Camera P1 ISP driver? Please make sure this is reasonably
> consistent with other drivers (SenInf, DIP, FD).
>
> > +MODULE_LICENSE("GPL");
>
> GPL v2
>
We will check this naming with other drivers & fix the version.
> diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.h
> new file mode 100644
> index 000000000000..6af3f569664c
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.h
> @@ -0,0 +1,243 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + */
> > +
> > +#ifndef __CAMERA_ISP_H
> > +#define __CAMERA_ISP_H
> > +
> > +#include <linux/cdev.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/ioctl.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/pm_qos.h>
> > +#include <linux/scatterlist.h>
> > +
> > +#include "mtk_cam-scp.h"
> > +#include "mtk_cam-v4l2-util.h"
> > +
> > +#define CAM_A_MAX_WIDTH 3328
> > +#define CAM_A_MAX_HEIGHT 2496
> > +#define CAM_B_MAX_WIDTH 5376
> > +#define CAM_B_MAX_HEIGHT 4032
> > +
> > +#define CAM_MIN_WIDTH 80
> > +#define CAM_MIN_HEIGHT 60
> > +
> > +#define IMG_MAX_WIDTH CAM_B_MAX_WIDTH
> > +#define IMG_MAX_HEIGHT CAM_B_MAX_HEIGHT
> > +#define IMG_MIN_WIDTH CAM_MIN_WIDTH
> > +#define IMG_MIN_HEIGHT CAM_MIN_HEIGHT
> > +
> > +#define RRZ_MAX_WIDTH CAM_B_MAX_WIDTH
> > +#define RRZ_MAX_HEIGHT CAM_B_MAX_HEIGHT
> > +#define RRZ_MIN_WIDTH CAM_MIN_WIDTH
> > +#define RRZ_MIN_HEIGHT CAM_MIN_HEIGHT
> > +
> > +#define R_IMGO BIT(0)
> > +#define R_RRZO BIT(1)
> > +#define R_AAO BIT(3)
> > +#define R_AFO BIT(4)
> > +#define R_LCSO BIT(5)
> > +#define R_PDO BIT(6)
> > +#define R_LMVO BIT(7)
> > +#define R_FLKO BIT(8)
> > +#define R_RSSO BIT(9)
> > +#define R_PSO BIT(10)
> > +
> > +#define CQ_BUFFER_COUNT 3
> > +#define IRQ_DATA_BUF_SIZE 4
> > +#define CQ_ADDRESS_OFFSET 0x640
> > +
> > +#define ISP_COMPOSING_MAX_NUM 4
> > +#define ISP_FRAME_COMPOSING_MAX_NUM 3
> > +
> > +#define IRQ_STAT_STR "cam%c, SOF_%d irq(0x%x), " \
> > + "dma(0x%x), frame_num(%d)/cq_num(%d), " \
> > + "fbc1(0x%x), fbc2(0x%x)\n"
> > +
> > +/*
> > + * In order with the sequence of device nodes defined in dtsi rule,
> > + * one hardware module should be mapping to one node.
> > + */
> > +enum isp_dev_node_enum {
> > + ISP_CAMSYS_CONFIG_IDX = 0,
> > + ISP_CAM_UNI_IDX,
> > + ISP_CAM_A_IDX,
> > + ISP_CAM_B_IDX,
> > + ISP_DEV_NODE_NUM
> > +};
> > +
> > +/* Image RAW path for ISP P1 module. */
> > +enum isp_raw_path_enum {
> > + ISP_PROCESS_RAW_PATH = 0,
> > + ISP_PURE_RAW_PATH
> > +};
> > +
> > +/* State for struct mtk_isp_p1_ctx: composer_state */
> > +enum {
> > + SCP_ON = 0,
> > + SCP_OFF
> > +};
>
> Hmm, looks like bool.
>
This will be removed in next patch.
> > +
> > +enum {
> > + IMG_FMT_UNKNOWN = 0x0000,
> > + IMG_FMT_RAW_START = 0x2200,
> > + IMG_FMT_BAYER8 = IMG_FMT_RAW_START,
> > + IMG_FMT_BAYER10,
> > + IMG_FMT_BAYER12,
> > + IMG_FMT_BAYER14,
> > + IMG_FMT_FG_BAYER8,
> > + IMG_FMT_FG_BAYER10,
> > + IMG_FMT_FG_BAYER12,
> > + IMG_FMT_FG_BAYER14,
> > +};
> > +
> > +enum {
> > + RAW_PXL_ID_B = 0,
> > + RAW_PXL_ID_GB,
> > + RAW_PXL_ID_GR,
> > + RAW_PXL_ID_R
> > +};
>
> Please use macros with explicitly assigned values for hardware/firmware ABI
> definitions.
>
Fix in next patch.
> > +
> > +struct isp_queue {
> > + struct list_head queue;
> > + atomic_t queue_cnt;
> > + spinlock_t lock; /* queue attributes protection */
> > +};
> > +
> > +struct isp_thread {
> > + struct task_struct *thread;
> > + wait_queue_head_t wq;
> > +};
> > +
> > +struct mtk_isp_queue_work {
> > + union {
> > + struct mtk_isp_scp_p1_cmd cmd;
> > + struct p1_frame_param frameparams;
> > + };
> > + struct list_head list_entry;
> > + enum mtk_isp_scp_type type;
> > +};
> > +
> > +struct mtk_cam_dev_stat_event_data {
> > + __u32 frame_seq_no;
> > + __u32 meta0_vb2_index;
> > + __u32 meta1_vb2_index;
> > + __u32 irq_status_mask;
> > + __u32 dma_status_mask;
> > +};
> > +
> > +struct mtk_isp_queue_job {
> > + struct list_head list_entry;
> > + struct list_head list_buf;
> > + unsigned int request_fd;
> > + unsigned int frame_seq_no;
> > +};
>
> Please document the structs above using kerneldoc.
>
These structures are removed in next patch.
> > +
> > +/*
> > + * struct isp_device - the ISP device information
> > + *
> > + * @dev: Pointer to struct device
> > + * @regs: Camera ISP base register address
> > + * @spinlock_irq: Used to protect register read/write data
> > + * @current_frame: Current frame sequence number, set when SOF
> > + * @meta0_vb2_index: Meta0 vb2 buffer index, set when SOF
> > + * @meta1_vb2_index: Meta1 vb2 buffer index, set when SOF
> > + * @sof_count: The accumulated SOF counter
> > + * @isp_hw_module: Identity camera A or B
> > + *
> > + */
> > +struct isp_device {
>
> mtk_isp_device?
>
Fix in next patch.
> > + struct device *dev;
> > + void __iomem *regs;
> > + spinlock_t spinlock_irq; /* ISP reg setting integrity */
> > + unsigned int current_frame;
> > + unsigned int meta0_vb2_index;
> > + unsigned int meta1_vb2_index;
> > + u8 sof_count;
> > + u8 isp_hw_module;
> > +};
> > +
> > +/*
> > + * struct mtk_isp_p1_ctx - the ISP device information
> > + *
> > + * @composer_txlist: Queue for SCP TX data including SCP_ISP_CMD & SCP_ISP_FRAME
> > + * @composer_tx_thread: TX Thread for SCP data tranmission
> > + * @cmd_queued: The number of SCP_ISP_CMD commands will be sent
> > + * @ipi_occupied: The total number of SCP TX data has beent sent
> > + * @scp_state: The state of SCP control
> > + * @composing_frame: The total number of SCP_ISP_FRAME has beent sent
> > + * @composed_frame_id: The ack. frame sequence by SCP
> > + * @composer_deinit_thread: The de-initialized thread
> > + * @p1_enqueue_list: Queue for ISP frame buffers
> > + * @isp_deque_thread: Thread for handling ISP frame buffers dequeue
> > + * @irq_event_datas: Ring buffer for struct mtk_cam_dev_stat_event_data data
> > + * @irq_data_start: Start index of irq_event_datas ring buffer
> > + * @irq_data_end: End index of irq_event_datas ring buffer
> > + * @irq_dequeue_lock: Lock to protect irq_event_datas ring buffer
> > + * @scp_mem_pa: DMA address for SCP device
> > + * @scp_mem_iova: DMA address for ISP HW DMA devices
> > + * @frame_seq_no: Sequence number for ISP frame buffer
> > + * @isp_hw_module: Active camera HW module
> > + * @num_clks: The number of driver's clock
> > + * @clk_list: The list of clock data
> > + * @lock: Lock to protect context operations
> > + *
> > + */
> > +struct mtk_isp_p1_ctx {
> > + struct isp_queue composer_txlist;
> > + struct isp_thread composer_tx_thread;
> > + atomic_t cmd_queued;
> > + atomic_t ipi_occupied;
> > + atomic_t scp_state;
> > + atomic_t composing_frame;
> > + atomic_t composed_frame_id;
> > + struct isp_thread composer_deinit_thread;
> > + struct isp_queue p1_enqueue_list;
> > + struct isp_thread isp_deque_thread;
> > + struct mtk_cam_dev_stat_event_data irq_event_datas[IRQ_DATA_BUF_SIZE];
> > + atomic_t irq_data_start;
> > + atomic_t irq_data_end;
> > + spinlock_t irq_dequeue_lock; /* ISP frame dequeuq protection */
>
> Already documented in kerneldoc.
>
Fix in next patch.
> > + dma_addr_t scp_mem_pa;
> > + dma_addr_t scp_mem_iova;
> > + int frame_seq_no;
> > + unsigned int isp_hw_module;
> > + unsigned int isp_raw_path;
>
> Not documented above.
>
Fix in next patch.
> > + unsigned int num_clks;
> > + struct clk_bulk_data *clk_list;
> > + struct mutex lock; /* Protect context operations */
>
> Already documented in kerneldoc.
>
Fix in next patch.
> > +};
> > +
> > +struct isp_p1_device {
> > + struct platform_device *pdev;
> > + struct platform_device *scp_pdev;
> > + struct rproc *rproc_handle;
> > + struct mtk_isp_p1_ctx isp_ctx;
> > + struct mtk_cam_dev cam_dev;
> > + struct isp_device isp_devs[ISP_DEV_NODE_NUM];
> > +};
>
> Please document in a kerneldoc comment.
>
Fix in next patch.
> > +
> > +static inline struct isp_p1_device *
> > +p1_ctx_to_dev(const struct mtk_isp_p1_ctx *__p1_ctx)
> > +{
> > + return container_of(__p1_ctx, struct isp_p1_device, isp_ctx);
> > +}
> > +
> > +static inline struct isp_p1_device *get_p1_device(struct device *dev)
> > +{
> > + return ((struct isp_p1_device *)dev_get_drvdata(dev));
>
> No need to cast. And, I don't think we need a function for this, just call
> dev_get_drvdata() directly.
>
Fix in next patch.
> Best regards,
> Tomasz
>
Thank you for your valuable comments .
Best regards,
Jungo
^ permalink raw reply
* Re: [PATCH v4 6/8] ARM: sunxi: dts: s3/s3l/v3: add DTSI files for S3/S3L/V3 SoCs
From: Icenowy Zheng @ 2019-07-20 9:50 UTC (permalink / raw)
To: linux-arm-kernel, Maxime Ripard
Cc: devicetree, Linus Walleij, linux-sunxi, linux-kernel, linux-gpio,
Chen-Yu Tsai, Rob Herring, linux-clk
In-Reply-To: <20190720094814.s6qik6ja2ympwpmp@flea>
于 2019年7月20日 GMT+08:00 下午5:48:14, Maxime Ripard <maxime.ripard@bootlin.com> 写到:
>On Sat, Jul 13, 2019 at 11:46:32AM +0800, Icenowy Zheng wrote:
>> The Allwinner S3/S3L/V3 SoCs all share the same die with the V3s SoC,
>> but with more GPIO wired out of the package.
>>
>> Add DTSI files for these SoCs. The DTSI file for V3 just replaces the
>> pinctrl compatible string, and the S3/S3L DTSI files just include the
>V3
>> DTSI file.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>> No changes since v2.
>>
>> arch/arm/boot/dts/sun8i-s3.dtsi | 6 ++++++
>> arch/arm/boot/dts/sun8i-s3l.dtsi | 6 ++++++
>> arch/arm/boot/dts/sun8i-v3.dtsi | 14 ++++++++++++++
>> 3 files changed, 26 insertions(+)
>> create mode 100644 arch/arm/boot/dts/sun8i-s3.dtsi
>> create mode 100644 arch/arm/boot/dts/sun8i-s3l.dtsi
>> create mode 100644 arch/arm/boot/dts/sun8i-v3.dtsi
>>
>> diff --git a/arch/arm/boot/dts/sun8i-s3.dtsi
>b/arch/arm/boot/dts/sun8i-s3.dtsi
>> new file mode 100644
>> index 000000000000..0f41a25ecb30
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/sun8i-s3.dtsi
>> @@ -0,0 +1,6 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2019 Icenowy Zheng <icenowy@aosc.io>
>> + */
>> +
>> +#include "sun8i-v3.dtsi"
>> diff --git a/arch/arm/boot/dts/sun8i-s3l.dtsi
>b/arch/arm/boot/dts/sun8i-s3l.dtsi
>> new file mode 100644
>> index 000000000000..0f41a25ecb30
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/sun8i-s3l.dtsi
>> @@ -0,0 +1,6 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2019 Icenowy Zheng <icenowy@aosc.io>
>> + */
>> +
>> +#include "sun8i-v3.dtsi"
>> diff --git a/arch/arm/boot/dts/sun8i-v3.dtsi
>b/arch/arm/boot/dts/sun8i-v3.dtsi
>> new file mode 100644
>> index 000000000000..6ae8645ade50
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/sun8i-v3.dtsi
>> @@ -0,0 +1,14 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2019 Icenowy Zheng <icenowy@aosc.io>
>> + */
>> +
>> +#include "sun8i-v3s.dtsi"
>> +
>> +&ccu {
>> + compatible = "allwinner,sun8i-v3-ccu";
>> +};
>> +
>> +&pio {
>> + compatible = "allwinner,sun8i-v3-pinctrl";
>> +};
>
>Is there any difference between the S3, S3L and V3?
DRAM. This might be useful when we introduce DRAM DVFS.
>
>If not, then we don't need all those DTSI, just add the v3
But I agree with this now.
>
>Maxime
>
>--
>Maxime Ripard, Bootlin
>Embedded Linux and Kernel engineering
>https://bootlin.com
--
使用 K-9 Mail 发送自我的Android设备。
^ permalink raw reply
* Re: [PATCH v4 6/8] ARM: sunxi: dts: s3/s3l/v3: add DTSI files for S3/S3L/V3 SoCs
From: Maxime Ripard @ 2019-07-20 9:48 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Rob Herring, Chen-Yu Tsai, Linus Walleij,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20190713034634.44585-7-icenowy-h8G6r0blFSE@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2296 bytes --]
On Sat, Jul 13, 2019 at 11:46:32AM +0800, Icenowy Zheng wrote:
> The Allwinner S3/S3L/V3 SoCs all share the same die with the V3s SoC,
> but with more GPIO wired out of the package.
>
> Add DTSI files for these SoCs. The DTSI file for V3 just replaces the
> pinctrl compatible string, and the S3/S3L DTSI files just include the V3
> DTSI file.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> No changes since v2.
>
> arch/arm/boot/dts/sun8i-s3.dtsi | 6 ++++++
> arch/arm/boot/dts/sun8i-s3l.dtsi | 6 ++++++
> arch/arm/boot/dts/sun8i-v3.dtsi | 14 ++++++++++++++
> 3 files changed, 26 insertions(+)
> create mode 100644 arch/arm/boot/dts/sun8i-s3.dtsi
> create mode 100644 arch/arm/boot/dts/sun8i-s3l.dtsi
> create mode 100644 arch/arm/boot/dts/sun8i-v3.dtsi
>
> diff --git a/arch/arm/boot/dts/sun8i-s3.dtsi b/arch/arm/boot/dts/sun8i-s3.dtsi
> new file mode 100644
> index 000000000000..0f41a25ecb30
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun8i-s3.dtsi
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2019 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + */
> +
> +#include "sun8i-v3.dtsi"
> diff --git a/arch/arm/boot/dts/sun8i-s3l.dtsi b/arch/arm/boot/dts/sun8i-s3l.dtsi
> new file mode 100644
> index 000000000000..0f41a25ecb30
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun8i-s3l.dtsi
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2019 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + */
> +
> +#include "sun8i-v3.dtsi"
> diff --git a/arch/arm/boot/dts/sun8i-v3.dtsi b/arch/arm/boot/dts/sun8i-v3.dtsi
> new file mode 100644
> index 000000000000..6ae8645ade50
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun8i-v3.dtsi
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2019 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + */
> +
> +#include "sun8i-v3s.dtsi"
> +
> +&ccu {
> + compatible = "allwinner,sun8i-v3-ccu";
> +};
> +
> +&pio {
> + compatible = "allwinner,sun8i-v3-pinctrl";
> +};
Is there any difference between the S3, S3L and V3?
If not, then we don't need all those DTSI, just add the v3
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v4 5/8] clk: sunxi-ng: v3s: add Allwinner V3 support
From: Maxime Ripard @ 2019-07-20 9:46 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Rob Herring, Chen-Yu Tsai, Linus Walleij, devicetree,
linux-arm-kernel, linux-kernel, linux-clk, linux-gpio,
linux-sunxi
In-Reply-To: <20190713034634.44585-6-icenowy@aosc.io>
[-- Attachment #1: Type: text/plain, Size: 484 bytes --]
On Sat, Jul 13, 2019 at 11:46:31AM +0800, Icenowy Zheng wrote:
> + [CLK_MMC1] = &mmc1_clk.common.hw,
> + [CLK_MMC1_SAMPLE] = &mmc1_sample_clk.common.hw,
> + [CLK_MMC1_OUTPUT] = &mmc1_output_clk.common.hw,
> + [CLK_MMC2] = &mmc1_clk.common.hw,
> + [CLK_MMC2_SAMPLE] = &mmc1_sample_clk.common.hw,
> + [CLK_MMC2_OUTPUT] = &mmc1_output_clk.common.hw,
And there's the same mistake here.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v4 4/8] clk: sunxi-ng: v3s: add missing clock slices for MMC2 module clocks
From: Icenowy Zheng @ 2019-07-20 9:45 UTC (permalink / raw)
To: Maxime Ripard
Cc: Rob Herring, Chen-Yu Tsai, Linus Walleij, devicetree,
linux-arm-kernel, linux-kernel, linux-clk, linux-gpio,
linux-sunxi
In-Reply-To: <20190720094449.dh53rbxz3mc74qls@flea>
于 2019年7月20日 GMT+08:00 下午5:44:49, Maxime Ripard <maxime.ripard@bootlin.com> 写到:
>On Sat, Jul 13, 2019 at 11:46:30AM +0800, Icenowy Zheng wrote:
>> The MMC2 clock slices are currently not defined in V3s CCU driver,
>which
>> makes MMC2 not working.
>>
>> Fix this issue.
>>
>> Fixes: d0f11d14b0bc ("clk: sunxi-ng: add support for V3s CCU")
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>> New patch in v4.
>>
>> drivers/clk/sunxi-ng/ccu-sun8i-v3s.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-v3s.c
>b/drivers/clk/sunxi-ng/ccu-sun8i-v3s.c
>> index 4eb68243e310..9c88015d4419 100644
>> --- a/drivers/clk/sunxi-ng/ccu-sun8i-v3s.c
>> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-v3s.c
>> @@ -513,6 +513,9 @@ static struct clk_hw_onecell_data
>sun8i_v3s_hw_clks = {
>> [CLK_MMC1] = &mmc1_clk.common.hw,
>> [CLK_MMC1_SAMPLE] = &mmc1_sample_clk.common.hw,
>> [CLK_MMC1_OUTPUT] = &mmc1_output_clk.common.hw,
>> + [CLK_MMC2] = &mmc1_clk.common.hw,
>> + [CLK_MMC2_SAMPLE] = &mmc1_sample_clk.common.hw,
>> + [CLK_MMC2_OUTPUT] = &mmc1_output_clk.common.hw,
>
>You're using the same structures than mmc1, I guess it's a copy and
>paste mistake?
Yes.
>
>Maxime
>
>--
>Maxime Ripard, Bootlin
>Embedded Linux and Kernel engineering
>https://bootlin.com
--
使用 K-9 Mail 发送自我的Android设备。
^ permalink raw reply
* Re: [PATCH v4 4/8] clk: sunxi-ng: v3s: add missing clock slices for MMC2 module clocks
From: Maxime Ripard @ 2019-07-20 9:44 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Rob Herring, Chen-Yu Tsai, Linus Walleij,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20190713034634.44585-5-icenowy-h8G6r0blFSE@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]
On Sat, Jul 13, 2019 at 11:46:30AM +0800, Icenowy Zheng wrote:
> The MMC2 clock slices are currently not defined in V3s CCU driver, which
> makes MMC2 not working.
>
> Fix this issue.
>
> Fixes: d0f11d14b0bc ("clk: sunxi-ng: add support for V3s CCU")
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> New patch in v4.
>
> drivers/clk/sunxi-ng/ccu-sun8i-v3s.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-v3s.c b/drivers/clk/sunxi-ng/ccu-sun8i-v3s.c
> index 4eb68243e310..9c88015d4419 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun8i-v3s.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-v3s.c
> @@ -513,6 +513,9 @@ static struct clk_hw_onecell_data sun8i_v3s_hw_clks = {
> [CLK_MMC1] = &mmc1_clk.common.hw,
> [CLK_MMC1_SAMPLE] = &mmc1_sample_clk.common.hw,
> [CLK_MMC1_OUTPUT] = &mmc1_output_clk.common.hw,
> + [CLK_MMC2] = &mmc1_clk.common.hw,
> + [CLK_MMC2_SAMPLE] = &mmc1_sample_clk.common.hw,
> + [CLK_MMC2_OUTPUT] = &mmc1_output_clk.common.hw,
You're using the same structures than mmc1, I guess it's a copy and
paste mistake?
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v4 3/8] dt-bindings: clk: sunxi-ccu: add compatible string for V3 CCU
From: Maxime Ripard @ 2019-07-20 9:44 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Rob Herring, Chen-Yu Tsai, Linus Walleij,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Rob Herring
In-Reply-To: <20190713034634.44585-4-icenowy-h8G6r0blFSE@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 602 bytes --]
On Sat, Jul 13, 2019 at 11:46:29AM +0800, Icenowy Zheng wrote:
> Despite Allwinner V3 and V3s shares the same die, one peripheral (I2S)
> is only available on V3, and thus the clocks is not declared for V3s
> CCU.
>
> Add a V3 CCU compatible string to the binding to prepare for a CCU
> driver that provide I2S clock on V3, but not on V3s.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Queued for 5.4, thanks
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v4 2/8] clk: sunxi-ng: v3s: add the missing PLL_DDR1
From: Maxime Ripard @ 2019-07-20 9:43 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Rob Herring, Chen-Yu Tsai, Linus Walleij,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20190713034634.44585-3-icenowy-h8G6r0blFSE@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 461 bytes --]
On Sat, Jul 13, 2019 at 11:46:28AM +0800, Icenowy Zheng wrote:
> The user manual of V3/V3s/S3 declares a PLL_DDR1, however it's forgot
> when developing the V3s CCU driver.
>
> Add back the missing PLL_DDR1.
>
> Fixes: d0f11d14b0bc ("clk: sunxi-ng: add support for V3s CCU")
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
queued for 5.4, thanks
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v6 11/22] clk: sunxi-ng: a64: Add minimum rate for PLL_MIPI
From: Michael Nazzareno Trimarchi @ 2019-07-20 9:42 UTC (permalink / raw)
To: Maxime Ripard
Cc: Jagan Teki, David Airlie, Daniel Vetter, Chen-Yu Tsai,
Michael Turquette, Rob Herring, Mark Rutland, linux-arm-kernel,
linux-kernel, linux-clk, dri-devel, devicetree, linux-amarula,
linux-sunxi
In-Reply-To: <20190720093202.6fn6xmhvsgawscnu@flea>
Hi
On Sat, Jul 20, 2019 at 11:32 AM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> On Sat, Jul 20, 2019 at 12:46:27PM +0530, Jagan Teki wrote:
> > On Sat, Jul 20, 2019 at 12:28 PM Maxime Ripard
> > <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Thu, Jul 11, 2019 at 07:43:16PM +0200, Michael Nazzareno Trimarchi wrote:
> > > > > > tcon-pixel clock is the rate that you want to achive on display side
> > > > > > and if you have 4 lanes 32bit or lanes and different bit number that
> > > > > > you need to have a clock that is able to put outside bits and speed
> > > > > > equal to pixel-clock * bits / lanes. so If you want a pixel-clock of
> > > > > > 40 mhz and you have 32bits and 4 lanes you need to have a clock of
> > > > > > 40 * 32 / 4 in no-burst mode. I think that this is done but most of
> > > > > > the display.
> > > > >
> > > > > So this is what the issue is then?
> > > > >
> > > > > This one does make sense, and you should just change the rate in the
> > > > > call to clk_set_rate in sun4i_tcon0_mode_set_cpu.
> > > > >
> > > > > I'm still wondering why that hasn't been brought up in either the
> > > > > discussion or the commit log before though.
> > > > >
> > > > Something like this?
> > > >
> > > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +++++++++++---------
> > > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 --
> > > > 2 files changed, 11 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > index 64c43ee6bd92..42560d5c327c 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > @@ -263,10 +263,11 @@ static int sun4i_tcon_get_clk_delay(const struct
> > > > drm_display_mode *mode,
> > > > }
> > > >
> > > > static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
> > > > - const struct drm_display_mode *mode)
> > > > + const struct drm_display_mode *mode,
> > > > + u32 tcon_mul)
> > > > {
> > > > /* Configure the dot clock */
> > > > - clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
> > > > + clk_set_rate(tcon->dclk, mode->crtc_clock * tcon_mul * 1000);
> > > >
> > > > /* Set the resolution */
> > > > regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
> > > > @@ -335,12 +336,13 @@ static void sun4i_tcon0_mode_set_cpu(struct
> > > > sun4i_tcon *tcon,
> > > > u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> > > > u8 lanes = device->lanes;
> > > > u32 block_space, start_delay;
> > > > - u32 tcon_div;
> > > > + u32 tcon_div, tcon_mul;
> > > >
> > > > - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> > > > - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
> > > > + tcon->dclk_min_div = 4;
> > > > + tcon->dclk_max_div = 127;
> > > >
> > > > - sun4i_tcon0_mode_set_common(tcon, mode);
> > > > + tcon_mul = bpp / lanes;
> > > > + sun4i_tcon0_mode_set_common(tcon, mode, tcon_mul);
> > > >
> > > > /* Set dithering if needed */
> > > > sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> > > > @@ -366,7 +368,7 @@ static void sun4i_tcon0_mode_set_cpu(struct
> > > > sun4i_tcon *tcon,
> > > > */
> > > > regmap_read(tcon->regs, SUN4I_TCON0_DCLK_REG, &tcon_div);
> > > > tcon_div &= GENMASK(6, 0);
> > > > - block_space = mode->htotal * bpp / (tcon_div * lanes);
> > > > + block_space = mode->htotal * tcon_div * tcon_mul;
> > > > block_space -= mode->hdisplay + 40;
> > > >
> > > > regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI0_REG,
> > > > @@ -408,7 +410,7 @@ static void sun4i_tcon0_mode_set_lvds(struct
> > > > sun4i_tcon *tcon,
> > > >
> > > > tcon->dclk_min_div = 7;
> > > > tcon->dclk_max_div = 7;
> > > > - sun4i_tcon0_mode_set_common(tcon, mode);
> > > > + sun4i_tcon0_mode_set_common(tcon, mode, 1);
> > > >
> > > > /* Set dithering if needed */
> > > > sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> > > > @@ -487,7 +489,7 @@ static void sun4i_tcon0_mode_set_rgb(struct
> > > > sun4i_tcon *tcon,
> > > >
> > > > tcon->dclk_min_div = 6;
> > > > tcon->dclk_max_div = 127;
> > > > - sun4i_tcon0_mode_set_common(tcon, mode);
> > > > + sun4i_tcon0_mode_set_common(tcon, mode, 1);
> > > >
> > > > /* Set dithering if needed */
> > > > sun4i_tcon0_mode_set_dithering(tcon, connector);
> > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > > index 5c3ad5be0690..a07090579f84 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > > @@ -13,8 +13,6 @@
> > > > #include <drm/drm_encoder.h>
> > > > #include <drm/drm_mipi_dsi.h>
> > > >
> > > > -#define SUN6I_DSI_TCON_DIV 4
> > > > -
> > > > struct sun6i_dsi {
> > > > struct drm_connector connector;
> > > > struct drm_encoder encoder;
> > >
> > > I had more something like this in mind:
> > > http://code.bulix.org/nlp5a4-803511
> >
> > Worth to look at it. was it working on your panel? meanwhile I will check it.
>
> I haven't tested it.
>
> > We have updated with below change [1], seems working on but is
> > actually checking the each divider as before start with 4... till 127.
> >
> > This new approach, is start looking the best divider from 4.. based on
> > the idea vs rounded it will ended up best divider like [2]
>
> But why?
>
> I mean, it's not like it's the first time I'm asking this...
>
> If the issue is what Micheal described, then the divider has nothing
> to do with it. We've had that discussion over and over again.
>
Sorry to quick copy and paste but my idea is somenthing like:
---
drivers/gpu/drm/sun4i/sun4i_dotclock.c | 5 ++++-
drivers/gpu/drm/sun4i/sun4i_tcon.c | 9 ++++++---
drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 --
4 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
index 417ade3d2565..b8be67cbb037 100644
--- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
+++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
@@ -74,12 +74,15 @@ static long sun4i_dclk_round_rate(struct clk_hw
*hw, unsigned long rate,
struct sun4i_tcon *tcon = dclk->tcon;
unsigned long best_parent = 0;
u8 best_div = 1;
+ u64 ideal = (u64)rate * tcon->dclk_mul;
int i;
for (i = tcon->dclk_min_div; i <= tcon->dclk_max_div; i++) {
- u64 ideal = (u64)rate * i;
unsigned long rounded;
+ if (!tcon->dclk_mul)
+ ideal = (u64)rate * i;
+
/*
* ideal has overflowed the max value that can be stored in an
* unsigned long, and every clk operation we might do on a
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 64c43ee6bd92..3301952bcf32 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -337,8 +337,9 @@ static void sun4i_tcon0_mode_set_cpu(struct
sun4i_tcon *tcon,
u32 block_space, start_delay;
u32 tcon_div;
- tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
- tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
+ tcon->dclk_min_div = 4;
+ tcon->dclk_max_div = 127;
+ tcon->dclk_mul = bpp / lanes;
sun4i_tcon0_mode_set_common(tcon, mode);
@@ -366,7 +367,7 @@ static void sun4i_tcon0_mode_set_cpu(struct
sun4i_tcon *tcon,
*/
regmap_read(tcon->regs, SUN4I_TCON0_DCLK_REG, &tcon_div);
tcon_div &= GENMASK(6, 0);
- block_space = mode->htotal * bpp / (tcon_div * lanes);
+ block_space = mode->htotal * tcon_div * tcon->dclk_mul;
block_space -= mode->hdisplay + 40;
regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI0_REG,
@@ -408,6 +409,7 @@ static void sun4i_tcon0_mode_set_lvds(struct
sun4i_tcon *tcon,
tcon->dclk_min_div = 7;
tcon->dclk_max_div = 7;
+ tcon->dclk_mul = 0;
sun4i_tcon0_mode_set_common(tcon, mode);
/* Set dithering if needed */
@@ -487,6 +489,7 @@ static void sun4i_tcon0_mode_set_rgb(struct
sun4i_tcon *tcon,
tcon->dclk_min_div = 6;
tcon->dclk_max_div = 127;
+ tcon->dclk_mul = 0;
sun4i_tcon0_mode_set_common(tcon, mode);
/* Set dithering if needed */
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h
b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index f9f1fe80b206..44b69533778e 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -248,6 +248,7 @@ struct sun4i_tcon {
struct clk *dclk;
u8 dclk_max_div;
u8 dclk_min_div;
+ u8 dclk_mul;
/* Reset control */
struct reset_control *lcd_rst;
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
index 5c3ad5be0690..a07090579f84 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -13,8 +13,6 @@
#include <drm/drm_encoder.h>
#include <drm/drm_mipi_dsi.h>
-#define SUN6I_DSI_TCON_DIV 4
-
struct sun6i_dsi {
struct drm_connector connector;
struct drm_encoder encoder;
--
2.17.1
Michael
> So you need to come with some argument and proof that the divider of
> that clock need to change. Otherwise, stop trying to make that happen:
> it won't.
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
--
| Michael Nazzareno Trimarchi Amarula Solutions BV |
| COO - Founder Cruquiuskade 47 |
| +31(0)851119172 Amsterdam 1018 AM NL |
| [`as] http://www.amarulasolutions.com |
^ permalink raw reply related
* Re: [PATCH v6 11/22] clk: sunxi-ng: a64: Add minimum rate for PLL_MIPI
From: Maxime Ripard @ 2019-07-20 9:32 UTC (permalink / raw)
To: Jagan Teki
Cc: Michael Nazzareno Trimarchi, David Airlie, Daniel Vetter,
Chen-Yu Tsai, Michael Turquette, Rob Herring, Mark Rutland,
linux-arm-kernel, linux-kernel, linux-clk, dri-devel, devicetree,
linux-amarula, linux-sunxi
In-Reply-To: <CAMty3ZDE1xiNgHVLihH378dY5szzkr14V-fwLZdvPs12tY+G1A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 5829 bytes --]
On Sat, Jul 20, 2019 at 12:46:27PM +0530, Jagan Teki wrote:
> On Sat, Jul 20, 2019 at 12:28 PM Maxime Ripard
> <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> wrote:
> >
> > On Thu, Jul 11, 2019 at 07:43:16PM +0200, Michael Nazzareno Trimarchi wrote:
> > > > > tcon-pixel clock is the rate that you want to achive on display side
> > > > > and if you have 4 lanes 32bit or lanes and different bit number that
> > > > > you need to have a clock that is able to put outside bits and speed
> > > > > equal to pixel-clock * bits / lanes. so If you want a pixel-clock of
> > > > > 40 mhz and you have 32bits and 4 lanes you need to have a clock of
> > > > > 40 * 32 / 4 in no-burst mode. I think that this is done but most of
> > > > > the display.
> > > >
> > > > So this is what the issue is then?
> > > >
> > > > This one does make sense, and you should just change the rate in the
> > > > call to clk_set_rate in sun4i_tcon0_mode_set_cpu.
> > > >
> > > > I'm still wondering why that hasn't been brought up in either the
> > > > discussion or the commit log before though.
> > > >
> > > Something like this?
> > >
> > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +++++++++++---------
> > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 --
> > > 2 files changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > index 64c43ee6bd92..42560d5c327c 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > @@ -263,10 +263,11 @@ static int sun4i_tcon_get_clk_delay(const struct
> > > drm_display_mode *mode,
> > > }
> > >
> > > static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
> > > - const struct drm_display_mode *mode)
> > > + const struct drm_display_mode *mode,
> > > + u32 tcon_mul)
> > > {
> > > /* Configure the dot clock */
> > > - clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
> > > + clk_set_rate(tcon->dclk, mode->crtc_clock * tcon_mul * 1000);
> > >
> > > /* Set the resolution */
> > > regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
> > > @@ -335,12 +336,13 @@ static void sun4i_tcon0_mode_set_cpu(struct
> > > sun4i_tcon *tcon,
> > > u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> > > u8 lanes = device->lanes;
> > > u32 block_space, start_delay;
> > > - u32 tcon_div;
> > > + u32 tcon_div, tcon_mul;
> > >
> > > - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> > > - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
> > > + tcon->dclk_min_div = 4;
> > > + tcon->dclk_max_div = 127;
> > >
> > > - sun4i_tcon0_mode_set_common(tcon, mode);
> > > + tcon_mul = bpp / lanes;
> > > + sun4i_tcon0_mode_set_common(tcon, mode, tcon_mul);
> > >
> > > /* Set dithering if needed */
> > > sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> > > @@ -366,7 +368,7 @@ static void sun4i_tcon0_mode_set_cpu(struct
> > > sun4i_tcon *tcon,
> > > */
> > > regmap_read(tcon->regs, SUN4I_TCON0_DCLK_REG, &tcon_div);
> > > tcon_div &= GENMASK(6, 0);
> > > - block_space = mode->htotal * bpp / (tcon_div * lanes);
> > > + block_space = mode->htotal * tcon_div * tcon_mul;
> > > block_space -= mode->hdisplay + 40;
> > >
> > > regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI0_REG,
> > > @@ -408,7 +410,7 @@ static void sun4i_tcon0_mode_set_lvds(struct
> > > sun4i_tcon *tcon,
> > >
> > > tcon->dclk_min_div = 7;
> > > tcon->dclk_max_div = 7;
> > > - sun4i_tcon0_mode_set_common(tcon, mode);
> > > + sun4i_tcon0_mode_set_common(tcon, mode, 1);
> > >
> > > /* Set dithering if needed */
> > > sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> > > @@ -487,7 +489,7 @@ static void sun4i_tcon0_mode_set_rgb(struct
> > > sun4i_tcon *tcon,
> > >
> > > tcon->dclk_min_div = 6;
> > > tcon->dclk_max_div = 127;
> > > - sun4i_tcon0_mode_set_common(tcon, mode);
> > > + sun4i_tcon0_mode_set_common(tcon, mode, 1);
> > >
> > > /* Set dithering if needed */
> > > sun4i_tcon0_mode_set_dithering(tcon, connector);
> > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > index 5c3ad5be0690..a07090579f84 100644
> > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > @@ -13,8 +13,6 @@
> > > #include <drm/drm_encoder.h>
> > > #include <drm/drm_mipi_dsi.h>
> > >
> > > -#define SUN6I_DSI_TCON_DIV 4
> > > -
> > > struct sun6i_dsi {
> > > struct drm_connector connector;
> > > struct drm_encoder encoder;
> >
> > I had more something like this in mind:
> > http://code.bulix.org/nlp5a4-803511
>
> Worth to look at it. was it working on your panel? meanwhile I will check it.
I haven't tested it.
> We have updated with below change [1], seems working on but is
> actually checking the each divider as before start with 4... till 127.
>
> This new approach, is start looking the best divider from 4.. based on
> the idea vs rounded it will ended up best divider like [2]
But why?
I mean, it's not like it's the first time I'm asking this...
If the issue is what Micheal described, then the divider has nothing
to do with it. We've had that discussion over and over again.
So you need to come with some argument and proof that the divider of
that clock need to change. Otherwise, stop trying to make that happen:
it won't.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* [PATCH v3 3/3] dt-bindings: dma: Convert Allwinner A31 and A64 DMA to a schema
From: Maxime Ripard @ 2019-07-20 9:26 UTC (permalink / raw)
To: Mark Rutland, Rob Herring, Frank Rowand, Vinod Koul
Cc: devicetree, Rob Herring, Maxime Ripard, Peter Ujfalusi,
Chen-Yu Tsai, dmaengine, linux-arm-kernel
In-Reply-To: <20190720092607.31095-1-maxime.ripard@bootlin.com>
The newer Allwinner SoCs have a DMA controller supported in Linux, with a
matching Device Tree binding.
Now that we have the DT validation in place, let's convert the device tree
bindings for that controller over to a YAML schemas.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
.../dma/allwinner,sun50i-a64-dma.yaml | 88 +++++++++++++++++++
.../bindings/dma/allwinner,sun6i-a31-dma.yaml | 62 +++++++++++++
.../devicetree/bindings/dma/sun6i-dma.txt | 81 -----------------
3 files changed, 150 insertions(+), 81 deletions(-)
create mode 100644 Documentation/devicetree/bindings/dma/allwinner,sun50i-a64-dma.yaml
create mode 100644 Documentation/devicetree/bindings/dma/allwinner,sun6i-a31-dma.yaml
delete mode 100644 Documentation/devicetree/bindings/dma/sun6i-dma.txt
diff --git a/Documentation/devicetree/bindings/dma/allwinner,sun50i-a64-dma.yaml b/Documentation/devicetree/bindings/dma/allwinner,sun50i-a64-dma.yaml
new file mode 100644
index 000000000000..4cb9d6b93138
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/allwinner,sun50i-a64-dma.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/allwinner,sun50i-a64-dma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner A64 DMA Controller Device Tree Bindings
+
+maintainers:
+ - Chen-Yu Tsai <wens@csie.org>
+ - Maxime Ripard <maxime.ripard@bootlin.com>
+
+allOf:
+ - $ref: "dma-controller.yaml#"
+
+properties:
+ "#dma-cells":
+ const: 1
+ description: The cell is the request line number.
+
+ compatible:
+ enum:
+ - allwinner,sun50i-a64-dma
+ - allwinner,sun50i-h6-dma
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ minItems: 1
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: bus
+ - const: mbus
+
+ resets:
+ maxItems: 1
+
+required:
+ - "#dma-cells"
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - resets
+ - dma-channels
+
+if:
+ properties:
+ compatible:
+ const: allwinner,sun50i-h6-dma
+
+then:
+ properties:
+ clocks:
+ maxItems: 2
+
+ required:
+ - clock-names
+
+else:
+ properties:
+ clocks:
+ maxItems: 1
+
+# FIXME: We should set it, but it would report all the generic
+# properties as additional properties.
+# additionalProperties: false
+
+examples:
+ - |
+ dma: dma-controller@1c02000 {
+ compatible = "allwinner,sun50i-a64-dma";
+ reg = <0x01c02000 0x1000>;
+ interrupts = <0 50 4>;
+ clocks = <&ccu 30>;
+ dma-channels = <8>;
+ dma-requests = <27>;
+ resets = <&ccu 7>;
+ #dma-cells = <1>;
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/dma/allwinner,sun6i-a31-dma.yaml b/Documentation/devicetree/bindings/dma/allwinner,sun6i-a31-dma.yaml
new file mode 100644
index 000000000000..740b7f9b535b
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/allwinner,sun6i-a31-dma.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/allwinner,sun6i-a31-dma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner A31 DMA Controller Device Tree Bindings
+
+maintainers:
+ - Chen-Yu Tsai <wens@csie.org>
+ - Maxime Ripard <maxime.ripard@bootlin.com>
+
+allOf:
+ - $ref: "dma-controller.yaml#"
+
+properties:
+ "#dma-cells":
+ const: 1
+ description: The cell is the request line number.
+
+ compatible:
+ oneOf:
+ - const: allwinner,sun6i-a31-dma
+ - const: allwinner,sun8i-a23-dma
+ - const: allwinner,sun8i-a83t-dma
+ - const: allwinner,sun8i-h3-dma
+ - const: allwinner,sun8i-v3s-dma
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+required:
+ - "#dma-cells"
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - resets
+
+additionalProperties: false
+
+examples:
+ - |
+ dma: dma-controller@1c02000 {
+ compatible = "allwinner,sun6i-a31-dma";
+ reg = <0x01c02000 0x1000>;
+ interrupts = <0 50 4>;
+ clocks = <&ahb1_gates 6>;
+ resets = <&ahb1_rst 6>;
+ #dma-cells = <1>;
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
deleted file mode 100644
index cae31f4e77ba..000000000000
--- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
+++ /dev/null
@@ -1,81 +0,0 @@
-Allwinner A31 DMA Controller
-
-This driver follows the generic DMA bindings defined in dma.txt.
-
-Required properties:
-
-- compatible: Must be one of
- "allwinner,sun6i-a31-dma"
- "allwinner,sun8i-a23-dma"
- "allwinner,sun8i-a83t-dma"
- "allwinner,sun8i-h3-dma"
- "allwinner,sun8i-v3s-dma"
-- reg: Should contain the registers base address and length
-- interrupts: Should contain a reference to the interrupt used by this device
-- clocks: Should contain a reference to the parent AHB clock
-- resets: Should contain a reference to the reset controller asserting
- this device in reset
-- #dma-cells : Should be 1, a single cell holding a line request number
-
-Example:
- dma: dma-controller@1c02000 {
- compatible = "allwinner,sun6i-a31-dma";
- reg = <0x01c02000 0x1000>;
- interrupts = <0 50 4>;
- clocks = <&ahb1_gates 6>;
- resets = <&ahb1_rst 6>;
- #dma-cells = <1>;
- };
-
-------------------------------------------------------------------------------
-For A64 and H6 DMA controller:
-
-Required properties:
-- compatible: Must be one of
- "allwinner,sun50i-a64-dma"
- "allwinner,sun50i-h6-dma"
-- dma-channels: Number of DMA channels supported by the controller.
- Refer to Documentation/devicetree/bindings/dma/dma.txt
-- clocks: In addition to parent AHB clock, it should also contain mbus
- clock (H6 only)
-- clock-names: Should contain "bus" and "mbus" (H6 only)
-- all properties above, i.e. reg, interrupts, clocks, resets and #dma-cells
-
-Optional properties:
-- dma-requests: Number of DMA request signals supported by the controller.
- Refer to Documentation/devicetree/bindings/dma/dma.txt
-
-Example:
- dma: dma-controller@1c02000 {
- compatible = "allwinner,sun50i-a64-dma";
- reg = <0x01c02000 0x1000>;
- interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&ccu CLK_BUS_DMA>;
- dma-channels = <8>;
- dma-requests = <27>;
- resets = <&ccu RST_BUS_DMA>;
- #dma-cells = <1>;
- };
-------------------------------------------------------------------------------
-
-Clients:
-
-DMA clients connected to the A31 DMA controller must use the format
-described in the dma.txt file, using a two-cell specifier for each
-channel: a phandle plus one integer cells.
-The two cells in order are:
-
-1. A phandle pointing to the DMA controller.
-2. The port ID as specified in the datasheet
-
-Example:
-spi2: spi@1c6a000 {
- compatible = "allwinner,sun6i-a31-spi";
- reg = <0x01c6a000 0x1000>;
- interrupts = <0 67 4>;
- clocks = <&ahb1_gates 22>, <&spi2_clk>;
- clock-names = "ahb", "mod";
- dmas = <&dma 25>, <&dma 25>;
- dma-names = "rx", "tx";
- resets = <&ahb1_rst 22>;
-};
--
2.21.0
^ permalink raw reply related
* [PATCH v3 2/3] dt-bindings: dma: Convert Allwinner A10 DMA to a schema
From: Maxime Ripard @ 2019-07-20 9:26 UTC (permalink / raw)
To: Mark Rutland, Rob Herring, Frank Rowand, Vinod Koul
Cc: devicetree, Rob Herring, Maxime Ripard, Peter Ujfalusi,
Chen-Yu Tsai, dmaengine, linux-arm-kernel
In-Reply-To: <20190720092607.31095-1-maxime.ripard@bootlin.com>
The older Allwinner SoCs have a DMA controller supported in Linux, with a
matching Device Tree binding.
Now that we have the DT validation in place, let's convert the device tree
bindings for that controller over to a YAML schemas.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
.../bindings/dma/allwinner,sun4i-a10-dma.yaml | 55 +++++++++++++++++++
.../devicetree/bindings/dma/sun4i-dma.txt | 45 ---------------
2 files changed, 55 insertions(+), 45 deletions(-)
create mode 100644 Documentation/devicetree/bindings/dma/allwinner,sun4i-a10-dma.yaml
delete mode 100644 Documentation/devicetree/bindings/dma/sun4i-dma.txt
diff --git a/Documentation/devicetree/bindings/dma/allwinner,sun4i-a10-dma.yaml b/Documentation/devicetree/bindings/dma/allwinner,sun4i-a10-dma.yaml
new file mode 100644
index 000000000000..15abc0f9429f
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/allwinner,sun4i-a10-dma.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/allwinner,sun4i-a10-dma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner A10 DMA Controller Device Tree Bindings
+
+maintainers:
+ - Chen-Yu Tsai <wens@csie.org>
+ - Maxime Ripard <maxime.ripard@bootlin.com>
+
+allOf:
+ - $ref: "dma-controller.yaml#"
+
+properties:
+ "#dma-cells":
+ const: 2
+ description:
+ The first cell is either 0 or 1, the former to use the normal
+ DMA, 1 for dedicated DMA. The second cell is the request line
+ number.
+
+ compatible:
+ const: allwinner,sun4i-a10-dma
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+required:
+ - "#dma-cells"
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+
+additionalProperties: false
+
+examples:
+ - |
+ dma: dma-controller@1c02000 {
+ compatible = "allwinner,sun4i-a10-dma";
+ reg = <0x01c02000 0x1000>;
+ interrupts = <27>;
+ clocks = <&ahb_gates 6>;
+ #dma-cells = <2>;
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/dma/sun4i-dma.txt b/Documentation/devicetree/bindings/dma/sun4i-dma.txt
deleted file mode 100644
index 8ad556aca70b..000000000000
--- a/Documentation/devicetree/bindings/dma/sun4i-dma.txt
+++ /dev/null
@@ -1,45 +0,0 @@
-Allwinner A10 DMA Controller
-
-This driver follows the generic DMA bindings defined in dma.txt.
-
-Required properties:
-
-- compatible: Must be "allwinner,sun4i-a10-dma"
-- reg: Should contain the registers base address and length
-- interrupts: Should contain a reference to the interrupt used by this device
-- clocks: Should contain a reference to the parent AHB clock
-- #dma-cells : Should be 2, first cell denoting normal or dedicated dma,
- second cell holding the request line number.
-
-Example:
- dma: dma-controller@1c02000 {
- compatible = "allwinner,sun4i-a10-dma";
- reg = <0x01c02000 0x1000>;
- interrupts = <27>;
- clocks = <&ahb_gates 6>;
- #dma-cells = <2>;
- };
-
-Clients:
-
-DMA clients connected to the Allwinner A10 DMA controller must use the
-format described in the dma.txt file, using a three-cell specifier for
-each channel: a phandle plus two integer cells.
-The three cells in order are:
-
-1. A phandle pointing to the DMA controller.
-2. Whether it is using normal (0) or dedicated (1) channels
-3. The port ID as specified in the datasheet
-
-Example:
- spi2: spi@1c17000 {
- compatible = "allwinner,sun4i-a10-spi";
- reg = <0x01c17000 0x1000>;
- interrupts = <0 12 4>;
- clocks = <&ahb_gates 22>, <&spi2_clk>;
- clock-names = "ahb", "mod";
- dmas = <&dma 1 29>, <&dma 1 28>;
- dma-names = "rx", "tx";
- #address-cells = <1>;
- #size-cells = <0>;
- };
--
2.21.0
^ permalink raw reply related
* [PATCH v3 1/3] dt-bindings: dma: Add YAML schemas for the generic DMA bindings
From: Maxime Ripard @ 2019-07-20 9:26 UTC (permalink / raw)
To: Mark Rutland, Rob Herring, Frank Rowand, Vinod Koul
Cc: devicetree, Maxime Ripard, Peter Ujfalusi, Chen-Yu Tsai,
dmaengine, linux-arm-kernel
The DMA controllers and consumers have a bunch of generic properties that
are needed in a device tree. Add a YAML schemas for those.
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
Changes from v2:
- Added a select statement.
Changes from v1:
- Dropped the dma consumer schemas
- Fixed the node name of the examples
- Enhanced a bit the description for dma-requests in case of a router
- Split the bindings in two to handle the router and controller case
separately
- Made #dma-cells required
---
.../devicetree/bindings/dma/dma-common.yaml | 45 +++++++
.../bindings/dma/dma-controller.yaml | 35 ++++++
.../devicetree/bindings/dma/dma-router.yaml | 50 ++++++++
Documentation/devicetree/bindings/dma/dma.txt | 114 +-----------------
4 files changed, 131 insertions(+), 113 deletions(-)
create mode 100644 Documentation/devicetree/bindings/dma/dma-common.yaml
create mode 100644 Documentation/devicetree/bindings/dma/dma-controller.yaml
create mode 100644 Documentation/devicetree/bindings/dma/dma-router.yaml
diff --git a/Documentation/devicetree/bindings/dma/dma-common.yaml b/Documentation/devicetree/bindings/dma/dma-common.yaml
new file mode 100644
index 000000000000..0141af047820
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/dma-common.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/dma-common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DMA Engine Generic Binding
+
+maintainers:
+ - Vinod Koul <vkoul@kernel.org>
+
+description:
+ Generic binding to provide a way for a driver using DMA Engine to
+ retrieve the DMA request or channel information that goes from a
+ hardware device to a DMA controller.
+
+select: false
+
+properties:
+ "#dma-cells":
+ minimum: 1
+ # Should be enough
+ maximum: 255
+ description:
+ Used to provide DMA controller specific information.
+
+ dma-channel-masks:
+ $ref: /schemas/types.yaml#definitions/uint32
+ description:
+ Bitmask of available DMA channels in ascending order that are
+ not reserved by firmware and are available to the
+ kernel. i.e. first channel corresponds to LSB.
+
+ dma-channels:
+ $ref: /schemas/types.yaml#definitions/uint32
+ description:
+ Number of DMA channels supported by the controller.
+
+ dma-requests:
+ $ref: /schemas/types.yaml#definitions/uint32
+ description:
+ Number of DMA request signals supported by the controller.
+
+required:
+ - "#dma-cells"
diff --git a/Documentation/devicetree/bindings/dma/dma-controller.yaml b/Documentation/devicetree/bindings/dma/dma-controller.yaml
new file mode 100644
index 000000000000..c39f6de76670
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/dma-controller.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/dma-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DMA Controller Generic Binding
+
+maintainers:
+ - Vinod Koul <vkoul@kernel.org>
+
+allOf:
+ - $ref: "dma-common.yaml#"
+
+# Everything else is described in the common file
+properties:
+ $nodename:
+ pattern: "^dma-controller(@.*)?$"
+
+examples:
+ - |
+ dma: dma-controller@48000000 {
+ compatible = "ti,omap-sdma";
+ reg = <0x48000000 0x1000>;
+ interrupts = <0 12 0x4
+ 0 13 0x4
+ 0 14 0x4
+ 0 15 0x4>;
+ #dma-cells = <1>;
+ dma-channels = <32>;
+ dma-requests = <127>;
+ dma-channel-mask = <0xfffe>;
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/dma/dma-router.yaml b/Documentation/devicetree/bindings/dma/dma-router.yaml
new file mode 100644
index 000000000000..5b5f07393135
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/dma-router.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/dma-router.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DMA Router Generic Binding
+
+maintainers:
+ - Vinod Koul <vkoul@kernel.org>
+
+allOf:
+ - $ref: "dma-common.yaml#"
+
+description:
+ DMA routers are transparent IP blocks used to route DMA request
+ lines from devices to the DMA controller. Some SoCs (like TI DRA7x)
+ have more peripherals integrated with DMA requests than what the DMA
+ controller can handle directly.
+
+properties:
+ $nodename:
+ pattern: "^dma-router(@.*)?$"
+
+ dma-masters:
+ $ref: /schemas/types.yaml#definitions/phandle-array
+ description:
+ Array of phandles to the DMA controllers the router can direct
+ the signal to.
+
+ dma-requests:
+ description:
+ Number of incoming request lines the router can handle.
+
+required:
+ - "#dma-cells"
+ - dma-masters
+
+examples:
+ - |
+ sdma_xbar: dma-router@4a002b78 {
+ compatible = "ti,dra7-dma-crossbar";
+ reg = <0x4a002b78 0xfc>;
+ #dma-cells = <1>;
+ dma-requests = <205>;
+ ti,dma-safe-map = <0>;
+ dma-masters = <&sdma>;
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/dma/dma.txt b/Documentation/devicetree/bindings/dma/dma.txt
index eeb4e4d1771e..90a67a016a48 100644
--- a/Documentation/devicetree/bindings/dma/dma.txt
+++ b/Documentation/devicetree/bindings/dma/dma.txt
@@ -1,113 +1 @@
-* Generic DMA Controller and DMA request bindings
-
-Generic binding to provide a way for a driver using DMA Engine to retrieve the
-DMA request or channel information that goes from a hardware device to a DMA
-controller.
-
-
-* DMA controller
-
-Required property:
-- #dma-cells: Must be at least 1. Used to provide DMA controller
- specific information. See DMA client binding below for
- more details.
-
-Optional properties:
-- dma-channels: Number of DMA channels supported by the controller.
-- dma-requests: Number of DMA request signals supported by the
- controller.
-- dma-channel-mask: Bitmask of available DMA channels in ascending order
- that are not reserved by firmware and are available to
- the kernel. i.e. first channel corresponds to LSB.
-
-Example:
-
- dma: dma@48000000 {
- compatible = "ti,omap-sdma";
- reg = <0x48000000 0x1000>;
- interrupts = <0 12 0x4
- 0 13 0x4
- 0 14 0x4
- 0 15 0x4>;
- #dma-cells = <1>;
- dma-channels = <32>;
- dma-requests = <127>;
- dma-channel-mask = <0xfffe>
- };
-
-* DMA router
-
-DMA routers are transparent IP blocks used to route DMA request lines from
-devices to the DMA controller. Some SoCs (like TI DRA7x) have more peripherals
-integrated with DMA requests than what the DMA controller can handle directly.
-
-Required property:
-- dma-masters: phandle of the DMA controller or list of phandles for
- the DMA controllers the router can direct the signal to.
-- #dma-cells: Must be at least 1. Used to provide DMA router specific
- information. See DMA client binding below for more
- details.
-
-Optional properties:
-- dma-requests: Number of incoming request lines the router can handle.
-- In the node pointed by the dma-masters:
- - dma-requests: The router driver might need to look for this in order
- to configure the routing.
-
-Example:
- sdma_xbar: dma-router@4a002b78 {
- compatible = "ti,dra7-dma-crossbar";
- reg = <0x4a002b78 0xfc>;
- #dma-cells = <1>;
- dma-requests = <205>;
- ti,dma-safe-map = <0>;
- dma-masters = <&sdma>;
- };
-
-* DMA client
-
-Client drivers should specify the DMA property using a phandle to the controller
-followed by DMA controller specific data.
-
-Required property:
-- dmas: List of one or more DMA specifiers, each consisting of
- - A phandle pointing to DMA controller node
- - A number of integer cells, as determined by the
- #dma-cells property in the node referenced by phandle
- containing DMA controller specific information. This
- typically contains a DMA request line number or a
- channel number, but can contain any data that is
- required for configuring a channel.
-- dma-names: Contains one identifier string for each DMA specifier in
- the dmas property. The specific strings that can be used
- are defined in the binding of the DMA client device.
- Multiple DMA specifiers can be used to represent
- alternatives and in this case the dma-names for those
- DMA specifiers must be identical (see examples).
-
-Examples:
-
-1. A device with one DMA read channel, one DMA write channel:
-
- i2c1: i2c@1 {
- ...
- dmas = <&dma 2 /* read channel */
- &dma 3>; /* write channel */
- dma-names = "rx", "tx";
- ...
- };
-
-2. A single read-write channel with three alternative DMA controllers:
-
- dmas = <&dma1 5
- &dma2 7
- &dma3 2>;
- dma-names = "rx-tx", "rx-tx", "rx-tx";
-
-3. A device with three channels, one of which has two alternatives:
-
- dmas = <&dma1 2 /* read channel */
- &dma1 3 /* write channel */
- &dma2 0 /* error read */
- &dma3 0>; /* alternative error read */
- dma-names = "rx", "tx", "error", "error";
+This file has been moved to dma-controller.yaml.
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 1/3] dt-bindings: dma: Add YAML schemas for the generic DMA bindings
From: Maxime Ripard @ 2019-07-20 9:24 UTC (permalink / raw)
To: Mark Rutland, Rob Herring, Frank Rowand, Vinod Koul
Cc: dmaengine, devicetree, Chen-Yu Tsai, linux-arm-kernel,
Peter Ujfalusi
In-Reply-To: <20190720064123.15411-1-maxime.ripard@bootlin.com>
[-- Attachment #1.1: Type: text/plain, Size: 2286 bytes --]
On Sat, Jul 20, 2019 at 08:41:21AM +0200, Maxime Ripard wrote:
> The DMA controllers and consumers have a bunch of generic properties that
> are needed in a device tree. Add a YAML schemas for those.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>
> ---
>
> Changes from v1:
> - Dropped the dma consumer schemas
> - Fixed the node name of the examples
> - Enhanced a bit the description for dma-requests in case of a router
> - Split the bindings in two to handle the router and controller case
> separately
> - Made #dma-cells required
> ---
> .../devicetree/bindings/dma/dma-common.yaml | 43 +++++++
> .../bindings/dma/dma-controller.yaml | 35 ++++++
> .../devicetree/bindings/dma/dma-router.yaml | 50 ++++++++
> Documentation/devicetree/bindings/dma/dma.txt | 114 +-----------------
> 4 files changed, 129 insertions(+), 113 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/dma/dma-common.yaml
> create mode 100644 Documentation/devicetree/bindings/dma/dma-controller.yaml
> create mode 100644 Documentation/devicetree/bindings/dma/dma-router.yaml
>
> diff --git a/Documentation/devicetree/bindings/dma/dma-common.yaml b/Documentation/devicetree/bindings/dma/dma-common.yaml
> new file mode 100644
> index 000000000000..422fd6c8b0ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/dma-common.yaml
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dma/dma-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: DMA Engine Generic Binding
> +
> +maintainers:
> + - Vinod Koul <vkoul@kernel.org>
> +
> +description:
> + Generic binding to provide a way for a driver using DMA Engine to
> + retrieve the DMA request or channel information that goes from a
> + hardware device to a DMA controller.
> +
> +properties:
> + "#dma-cells":
> + minimum: 1
> + # Should be enough
> + maximum: 255
> + description:
> + Used to provide DMA controller specific information.
We need a select statement here, otherwise it will try to validate
every node and report dma-cells as missing. I'll send a v3.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/8] pinctrl: sunxi: v3s: introduce support for V3
From: Maxime Ripard @ 2019-07-20 9:20 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Rob Herring, Chen-Yu Tsai, Linus Walleij,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20190713034634.44585-2-icenowy-h8G6r0blFSE@public.gmane.org>
On Sat, Jul 13, 2019 at 11:46:27AM +0800, Icenowy Zheng wrote:
> Introduce the GPIO pins that is only available on V3 (not on V3s) to the
> V3s pinctrl driver.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH 1/3] media: dt-bindings: Update bindings for Cadence CSI2TX version 2.1
From: Maxime Ripard @ 2019-07-20 8:26 UTC (permalink / raw)
To: Jan Kotas
Cc: mchehab, robh+dt, mark.rutland, linux-media, devicetree,
linux-kernel
In-Reply-To: <20190718111509.29924-2-jank@cadence.com>
[-- Attachment #1: Type: text/plain, Size: 337 bytes --]
On Thu, Jul 18, 2019 at 12:15:07PM +0100, Jan Kotas wrote:
> This patch adds a DT bindings documentation for
> Cadence CSI2TX v2.1 controller.
>
> Signed-off-by: Jan Kotas <jank@cadence.com>
Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] media: Add lane checks for Cadence CSI2TX
From: Maxime Ripard @ 2019-07-20 8:26 UTC (permalink / raw)
To: Jan Kotas
Cc: mchehab, robh+dt, mark.rutland, linux-media, devicetree,
linux-kernel
In-Reply-To: <20190718111509.29924-3-jank@cadence.com>
[-- Attachment #1: Type: text/plain, Size: 357 bytes --]
65;5603;1c
On Thu, Jul 18, 2019 at 12:15:08PM +0100, Jan Kotas wrote:
> This patch adds line checks for CSI2TX, to prevent
> clock lane being used as a data lane.
>
> Signed-off-by: Jan Kotas <jank@cadence.com>
Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] media: Add support for Cadence CSI2TX 2.1
From: Maxime Ripard @ 2019-07-20 8:25 UTC (permalink / raw)
To: Jan Kotas
Cc: mchehab, robh+dt, mark.rutland, linux-media, devicetree,
linux-kernel
In-Reply-To: <20190718111509.29924-4-jank@cadence.com>
[-- Attachment #1: Type: text/plain, Size: 4320 bytes --]
Hi,
On Thu, Jul 18, 2019 at 12:15:09PM +0100, Jan Kotas wrote:
> This patch adds support for CSI2TX v2.1 version of the controller.
>
> Signed-off-by: Jan Kotas <jank@cadence.com>
> ---
> drivers/media/platform/cadence/cdns-csi2tx.c | 142 +++++++++++++++++++++------
> 1 file changed, 112 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/platform/cadence/cdns-csi2tx.c b/drivers/media/platform/cadence/cdns-csi2tx.c
> index 232259c71..e4d08acfb 100644
> --- a/drivers/media/platform/cadence/cdns-csi2tx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2tx.c
> @@ -52,6 +52,17 @@
> #define CSI2TX_STREAM_IF_CFG_REG(n) (0x100 + (n) * 4)
> #define CSI2TX_STREAM_IF_CFG_FILL_LEVEL(n) ((n) & 0x1f)
>
> +/* CSI2TX V2 Registers */
> +#define CSI2TX_V2_DPHY_CFG_REG 0x28
> +#define CSI2TX_V2_DPHY_CFG_RESET BIT(16)
> +#define CSI2TX_V2_DPHY_CFG_CLOCK_MODE BIT(10)
> +#define CSI2TX_V2_DPHY_CFG_MODE_MASK GENMASK(9, 8)
> +#define CSI2TX_V2_DPHY_CFG_MODE_LPDT (2 << 8)
> +#define CSI2TX_V2_DPHY_CFG_MODE_HS (1 << 8)
> +#define CSI2TX_V2_DPHY_CFG_MODE_ULPS (0 << 8)
> +#define CSI2TX_V2_DPHY_CFG_CLK_ENABLE BIT(4)
> +#define CSI2TX_V2_DPHY_CFG_LANE_ENABLE(n) BIT(n)
> +
> #define CSI2TX_LANES_MAX 4
> #define CSI2TX_STREAMS_MAX 4
>
> @@ -70,6 +81,13 @@ struct csi2tx_fmt {
> u32 bpp;
> };
>
> +struct csi2tx_priv;
> +
> +/* CSI2TX Variant Operations */
> +struct csi2tx_vops {
> + void (*dphy_setup)(struct csi2tx_priv *csi2tx);
> +};
> +
> struct csi2tx_priv {
> struct device *dev;
> unsigned int count;
> @@ -82,6 +100,8 @@ struct csi2tx_priv {
>
> void __iomem *base;
>
> + struct csi2tx_vops *vops;
> +
> struct clk *esc_clk;
> struct clk *p_clk;
> struct clk *pixel_clk[CSI2TX_STREAMS_MAX];
> @@ -209,53 +229,92 @@ static const struct v4l2_subdev_pad_ops csi2tx_pad_ops = {
> .set_fmt = csi2tx_set_pad_format,
> };
>
> -static void csi2tx_reset(struct csi2tx_priv *csi2tx)
> +/* Set Wake Up value in the D-PHY */
> +static void csi2tx_dphy_set_wakeup(struct csi2tx_priv *csi2tx)
> {
> - writel(CSI2TX_CONFIG_SRST_REQ, csi2tx->base + CSI2TX_CONFIG_REG);
> -
> - udelay(10);
> + writel(CSI2TX_DPHY_CLK_WAKEUP_ULPS_CYCLES(32),
> + csi2tx->base + CSI2TX_DPHY_CLK_WAKEUP_REG);
> }
>
> -static int csi2tx_start(struct csi2tx_priv *csi2tx)
> +/*
> + * Finishes the D-PHY initialization
> + * reg dphy cfg value to be used
> + */
> +static void csi2tx_dphy_init_finish(struct csi2tx_priv *csi2tx, u32 reg)
> {
> - struct media_entity *entity = &csi2tx->subdev.entity;
> - struct media_link *link;
> unsigned int i;
> - u32 reg;
>
> - csi2tx_reset(csi2tx);
> + udelay(10);
>
> - writel(CSI2TX_CONFIG_CFG_REQ, csi2tx->base + CSI2TX_CONFIG_REG);
> + /* Enable our (clock and data) lanes */
> + reg |= CSI2TX_DPHY_CFG_CLK_ENABLE;
> + for (i = 0; i < csi2tx->num_lanes; i++)
> + reg |= CSI2TX_DPHY_CFG_LANE_ENABLE(csi2tx->lanes[i] - 1);
> + writel(reg, csi2tx->base + CSI2TX_DPHY_CFG_REG);
>
> udelay(10);
>
> - /* Configure our PPI interface with the D-PHY */
> - writel(CSI2TX_DPHY_CLK_WAKEUP_ULPS_CYCLES(32),
> - csi2tx->base + CSI2TX_DPHY_CLK_WAKEUP_REG);
> + /* Switch to HS mode */
> + reg &= ~CSI2TX_DPHY_CFG_MODE_MASK;
> + writel(reg | CSI2TX_DPHY_CFG_MODE_HS,
> + csi2tx->base + CSI2TX_DPHY_CFG_REG);
> +}
> +
> +/* Configures D-PHY in CSIv1.3 */
> +static void csi2tx_dphy_setup(struct csi2tx_priv *csi2tx)
> +{
> + u32 reg;
> + unsigned int i;
> +
> + csi2tx_dphy_set_wakeup(csi2tx);
>
> /* Put our lanes (clock and data) out of reset */
> reg = CSI2TX_DPHY_CFG_CLK_RESET | CSI2TX_DPHY_CFG_MODE_LPDT;
> for (i = 0; i < csi2tx->num_lanes; i++)
> - reg |= CSI2TX_DPHY_CFG_LANE_RESET(csi2tx->lanes[i]);
> + reg |= CSI2TX_DPHY_CFG_LANE_RESET(csi2tx->lanes[i] - 1);
This looks like a separate change?
> writel(reg, csi2tx->base + CSI2TX_DPHY_CFG_REG);
>
> - udelay(10);
> + csi2tx_dphy_init_finish(csi2tx, reg);
> +}
>
> - /* Enable our (clock and data) lanes */
> - reg |= CSI2TX_DPHY_CFG_CLK_ENABLE;
> - for (i = 0; i < csi2tx->num_lanes; i++)
> - reg |= CSI2TX_DPHY_CFG_LANE_ENABLE(csi2tx->lanes[i]);
And you have a similar change here that should be in a separate patch,
with a proper explanation.
Otherwise, the rest looks good.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [v5 1/2] dt-bindings: gpio: aspeed: Add SGPIO support
From: Linus Walleij @ 2019-07-20 8:12 UTC (permalink / raw)
To: Hongwei Zhang
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-aspeed, Bartosz Golaszewski, Andrew Jeffery,
linux-kernel@vger.kernel.org, open list:GPIO SUBSYSTEM,
Rob Herring, Joel Stanley, Linux ARM
In-Reply-To: <1563564291-9692-2-git-send-email-hongweiz@ami.com>
Hi Hongwei,
after looking close at the driver and bindings I have this feeback:
On Fri, Jul 19, 2019 at 9:25 PM Hongwei Zhang <hongweiz@ami.com> wrote:
+- reg : Address and length of the register set for the device
This 0x100 range may look simple but in the driver it looks like
this:
+static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
+ {
+ .val_regs = 0x0000,
+ .rdata_reg = 0x0070,
+ .irq_regs = 0x0004,
+ .names = { "A", "B", "C", "D" },
+ },
+ {
+ .val_regs = 0x001C,
+ .rdata_reg = 0x0074,
+ .irq_regs = 0x0020,
+ .names = { "E", "F", "G", "H" },
+ },
+ {
+ .val_regs = 0x0038,
+ .rdata_reg = 0x0078,
+ .irq_regs = 0x003C,
+ .names = { "I", "J" },
+ },
+};
So first break this into up to 10 different instances with one device
per bank instead.
For each device:
reg = <0x1e780200 4>, <x1e780204 ..>, <0x1e780270 ..>;
reg-names = "val", "irq", "rdata";
This way you use the device tree features for locating the
register offsets instead of hard-coding it into the driver,
which will make the driver simpler.
> +- ngpios : number of GPIO pins to serialise.
> + (should be multiple of 8, up to 80 pins)
This is wrong. This should be split into 10 different devices providing
8 GPIO lines each. The "ngpios" is not intended to subdivide
banks, but for the case where you use say bits 0..11 of 32 bits in
a register by setting ngpios to 12.
I see that they use the same clock divider and the same interrupt,
but this is better to partition into a separate clock divider driver
and up to 10 instances of GPIO devices with 8 GPIOs each.
I also see that they use the same interrupt. This is fine, in your
driver just grab a shared IRQ and all the IRQ handlers will be
traversed. This is a well known pattern for all operating systems.
> +- clocks : A phandle to the APB clock for SGPM clock division
> +
> +- bus-frequency : SGPM CLK frequency
I see that there is a common clock control register in offset 0x54.
Split this out to a separate clock driver that provides a divided clock
that all GPIO blocks can get, no matter if you use 1, 2 .. 10 of these
blocks.
The fact that these GPIO banks and the clock register is in the same
memory range does not really matter. Split up the memory range in
on reg = per GPIO chip and one reg for the clock.
> + Example:
> + sgpio: sgpio@1e780200 {
> + #gpio-cells = <2>;
> + compatible = "aspeed,ast2500-sgpio";
> + gpio-controller;
> + interrupts = <40>;
> + reg = <0x1e780200 0x0100>;
> + clocks = <&syscon ASPEED_CLK_APB>;
> + interrupt-controller;
> + ngpios = <8>;
> + bus-frequency = <12000000>;
> + };
Splitting this up into a clock controller and 1-10 instances of sgpio
will make things simpler, because it will closer reflect what the hardware
people are doing: they have just created 10 instances of the same
block, and added a clock divider.
You can put the 1-10 instances and the clock divider inside a collected
node "simple-bus" in the device tree:
{
compatible = "simple-bus";
sgpio0: sgpio {
compatible = "aspeed,ast2500-sgpio";
reg = <0x1e780200 ...> ...;
clk = <&sgpioclk>;
};
sgpio1: sgpio {
...
};
...
sgpioclk: clock {
compatible = "aspeed,sgpio-clock";
reg = 0x1e780254 4>;
};
};
This is a better fit with the actual hardware and will make it much
easier to write drivers.
Admittedly DT device partitioning of SoC devices is a grey area,
but here I think the DT infrastructure helps you to break things
down and make it easier, I am thinking in a divide-and-conquer
way about it.
Yours,
Linus Walleij
^ 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