[0/6] media: Introduce Allwinner H3 deinterlace driver
mbox series

Message ID 20190912175132.411-1-jernej.skrabec@siol.net
Headers show
Series
  • media: Introduce Allwinner H3 deinterlace driver
Related show

Message

Jernej Skrabec Sept. 12, 2019, 5:51 p.m. UTC
Starting with H3, Allwinner began to include standalone deinterlace
core in multimedia oriented SoCs. This patch series introduces support
for it. Note that new SoCs, like H6, have radically different (updated)
deinterlace core, which will need a new driver.

v4l2-compliance report:
v4l2-compliance SHA: dece02f862f38d8f866230ca9f1015cb93ddfac4, 32 bits

Compliance test for sun8i-di device /dev/video0:

Driver Info:
        Driver name      : sun8i-di
        Card type        : sun8i-di
        Bus info         : platform:sun8i-di
        Driver version   : 5.3.0
        Capabilities     : 0x84208000
                Video Memory-to-Memory
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x04208000
                Video Memory-to-Memory
                Streaming
                Extended Pix Format

Required ioctls:
        test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
        test second /dev/video0 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
        test for unlimited opens: OK

Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
        test VIDIOC_QUERYCTRL: OK (Not Supported)
        test VIDIOC_G/S_CTRL: OK (Not Supported)
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 0 Private Controls: 0

Format ioctls:
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
        test VIDIOC_G/S_PARM: OK (Not Supported)
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK
        test VIDIOC_TRY_FMT: OK
        test VIDIOC_S_FMT: OK
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK

Codec ioctls:
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
        test VIDIOC_EXPBUF: OK
        test Requests: OK (Not Supported)

Total for sun8i-di device /dev/video0: 44, Succeeded: 44, Failed: 0, Warnings: 0

Please take a look.

Best regards,
Jernej

Jernej Skrabec (6):
  dt-bindings: bus: sunxi: Add H3 MBUS compatible
  clk: sunxi-ng: h3: Export MBUS clock
  ARM: dts: sunxi: h3/h5: Add MBUS controller node
  dt-bindings: media: Add Allwinner H3 Deinterlace binding
  media: sun4i: Add H3 deinterlace driver
  dts: arm: sun8i: h3: Enable deinterlace unit

 .../bindings/arm/sunxi/sunxi-mbus.txt         |   1 +
 .../media/allwinner,sun8i-h3-deinterlace.yaml |  76 ++
 MAINTAINERS                                   |   7 +
 arch/arm/boot/dts/sun8i-h3.dtsi               |  13 +
 arch/arm/boot/dts/sunxi-h3-h5.dtsi            |   9 +
 drivers/clk/sunxi-ng/ccu-sun8i-h3.h           |   4 -
 drivers/media/platform/sunxi/Kconfig          |   1 +
 drivers/media/platform/sunxi/Makefile         |   1 +
 drivers/media/platform/sunxi/sun8i-di/Kconfig |  10 +
 .../media/platform/sunxi/sun8i-di/Makefile    |   2 +
 .../media/platform/sunxi/sun8i-di/sun8i-di.c  | 969 ++++++++++++++++++
 .../media/platform/sunxi/sun8i-di/sun8i-di.h  | 238 +++++
 include/dt-bindings/clock/sun8i-h3-ccu.h      |   2 +-
 13 files changed, 1328 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun8i-h3-deinterlace.yaml
 create mode 100644 drivers/media/platform/sunxi/sun8i-di/Kconfig
 create mode 100644 drivers/media/platform/sunxi/sun8i-di/Makefile
 create mode 100644 drivers/media/platform/sunxi/sun8i-di/sun8i-di.c
 create mode 100644 drivers/media/platform/sunxi/sun8i-di/sun8i-di.h

Comments

Maxime Ripard Sept. 12, 2019, 8:20 p.m. UTC | #1
Hi,

On Thu, Sep 12, 2019 at 07:51:29PM +0200, Jernej Skrabec wrote:
> Both, H3 and H5, contain MBUS, which is the bus used by DMA devices to
> access system memory.
> 
> MBUS controller is responsible for arbitration between channels based
> on set priority and can do some other things as well, like report
> bandwidth used. It also maps RAM region to different address than CPU.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  arch/arm/boot/dts/sunxi-h3-h5.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> index eba190b3f9de..ef1d03812636 100644
> --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> @@ -109,6 +109,7 @@
>  		compatible = "simple-bus";
>  		#address-cells = <1>;
>  		#size-cells = <1>;
> +		dma-ranges;
>  		ranges;
>  
>  		display_clocks: clock@1000000 {
> @@ -538,6 +539,14 @@
>  			};
>  		};
>  
> +		mbus: dram-controller@1c62000 {
> +			compatible = "allwinner,sun8i-h3-mbus";
> +			reg = <0x01c62000 0x1000>;
> +			clocks = <&ccu 113>;
> +			dma-ranges = <0x00000000 0x40000000 0xc0000000>;
> +			#interconnect-cells = <1>;
> +		};
> +

If that's easy enough to access, can you also add the references in
the devices that are already there? (CSI and DE comes to my mind, but
there might be others).

Thanks!
Maxime
Maxime Ripard Sept. 12, 2019, 8:26 p.m. UTC | #2
Hi,

On Thu, Sep 12, 2019 at 07:51:31PM +0200, Jernej Skrabec wrote:
> +	dev->regmap = devm_regmap_init_mmio(dev->dev, dev->base,
> +					    &deinterlace_regmap_config);
> +	if (IS_ERR(dev->regmap)) {
> +		dev_err(dev->dev, "Couldn't create deinterlace regmap\n");
> +
> +		return PTR_ERR(dev->regmap);
> +	}
> +
> +	ret = clk_prepare_enable(dev->bus_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable bus clock\n");
> +
> +		return ret;
> +	}

Do you need to keep the bus clock enabled all the time? Usually, for
the SoCs that have a reset line, you only need it to read / write to
the registers, not to have the controller actually running.

If you don't, then regmap_init_mmio_clk will take care of that for
you.

> +	clk_set_rate(dev->mod_clk, 300000000);
> +
> +	ret = clk_prepare_enable(dev->mod_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable mod clock\n");
> +
> +		goto err_bus_clk;
> +	}
> +
> +	ret = clk_prepare_enable(dev->ram_clk);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to enable ram clock\n");
> +
> +		goto err_mod_clk;
> +	}
> +
> +	ret = reset_control_reset(dev->rstc);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to apply reset\n");
> +
> +		goto err_ram_clk;
> +	}

This could be moved to a runtime_pm hook, with get_sync called in the
open. That way you won't leave the device powered on if it's unused.

> +struct deinterlace_dev {
> +	struct v4l2_device	v4l2_dev;
> +	struct video_device	vfd;
> +	struct device		*dev;
> +	struct v4l2_m2m_dev	*m2m_dev;
> +
> +	/* Device file mutex */
> +	struct mutex		dev_mutex;
> +
> +	void __iomem		*base;
> +	struct regmap		*regmap;

Do you need to store the base address in that structure if you're
using the regmap?

Maxime
Jernej Skrabec Sept. 12, 2019, 8:28 p.m. UTC | #3
Dne četrtek, 12. september 2019 ob 22:20:57 CEST je Maxime Ripard napisal(a):
> Hi,
> 
> On Thu, Sep 12, 2019 at 07:51:29PM +0200, Jernej Skrabec wrote:
> > Both, H3 and H5, contain MBUS, which is the bus used by DMA devices to
> > access system memory.
> > 
> > MBUS controller is responsible for arbitration between channels based
> > on set priority and can do some other things as well, like report
> > bandwidth used. It also maps RAM region to different address than CPU.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  arch/arm/boot/dts/sunxi-h3-h5.dtsi | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > b/arch/arm/boot/dts/sunxi-h3-h5.dtsi index eba190b3f9de..ef1d03812636
> > 100644
> > --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > @@ -109,6 +109,7 @@
> > 
> >  		compatible = "simple-bus";
> >  		#address-cells = <1>;
> >  		#size-cells = <1>;
> > 
> > +		dma-ranges;
> > 
> >  		ranges;
> >  		
> >  		display_clocks: clock@1000000 {
> > 
> > @@ -538,6 +539,14 @@
> > 
> >  			};
> >  		
> >  		};
> > 
> > +		mbus: dram-controller@1c62000 {
> > +			compatible = "allwinner,sun8i-h3-mbus";
> > +			reg = <0x01c62000 0x1000>;
> > +			clocks = <&ccu 113>;
> > +			dma-ranges = <0x00000000 0x40000000 
0xc0000000>;
> > +			#interconnect-cells = <1>;
> > +		};
> > +
> 
> If that's easy enough to access, can you also add the references in
> the devices that are already there? (CSI and DE comes to my mind, but
> there might be others).

Strangely, DE2 doesn't use this offset. That was tested on OrangePi Plus2E, 
which has 2 GiB of RAM and subtracting this offset causes corrupted image.

But I can add this properties to CSI too. However, wouldn't that need CSI DT 
binding expansion with those properties? othetwise DT check will fail.

Best regards,
Jernej

> 
> Thanks!
> Maxime
Maxime Ripard Sept. 12, 2019, 8:34 p.m. UTC | #4
On Thu, Sep 12, 2019 at 10:28:37PM +0200, Jernej Škrabec wrote:
> Dne četrtek, 12. september 2019 ob 22:20:57 CEST je Maxime Ripard napisal(a):
> > Hi,
> > 
> > On Thu, Sep 12, 2019 at 07:51:29PM +0200, Jernej Skrabec wrote:
> > > Both, H3 and H5, contain MBUS, which is the bus used by DMA devices to
> > > access system memory.
> > > 
> > > MBUS controller is responsible for arbitration between channels based
> > > on set priority and can do some other things as well, like report
> > > bandwidth used. It also maps RAM region to different address than CPU.
> > > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > ---
> > > 
> > >  arch/arm/boot/dts/sunxi-h3-h5.dtsi | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > b/arch/arm/boot/dts/sunxi-h3-h5.dtsi index eba190b3f9de..ef1d03812636
> > > 100644
> > > --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > @@ -109,6 +109,7 @@
> > > 
> > >  		compatible = "simple-bus";
> > >  		#address-cells = <1>;
> > >  		#size-cells = <1>;
> > > 
> > > +		dma-ranges;
> > > 
> > >  		ranges;
> > >  		
> > >  		display_clocks: clock@1000000 {
> > > 
> > > @@ -538,6 +539,14 @@
> > > 
> > >  			};
> > >  		
> > >  		};
> > > 
> > > +		mbus: dram-controller@1c62000 {
> > > +			compatible = "allwinner,sun8i-h3-mbus";
> > > +			reg = <0x01c62000 0x1000>;
> > > +			clocks = <&ccu 113>;
> > > +			dma-ranges = <0x00000000 0x40000000 
> 0xc0000000>;
> > > +			#interconnect-cells = <1>;
> > > +		};
> > > +
> > 
> > If that's easy enough to access, can you also add the references in
> > the devices that are already there? (CSI and DE comes to my mind, but
> > there might be others).
> 
> Strangely, DE2 doesn't use this offset. That was tested on OrangePi Plus2E, 
> which has 2 GiB of RAM and subtracting this offset causes corrupted image.

Ok, weird. But if it was tested then fine by me :)

> But I can add this properties to CSI too. However, wouldn't that need CSI DT 
> binding expansion with those properties? othetwise DT check will fail.

Oh right, we definitely need to update the binding indeed. The code
should be able to cope with both cases already.

Maxime
Jernej Skrabec Sept. 12, 2019, 8:43 p.m. UTC | #5
Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a):
> Hi,
> 
> On Thu, Sep 12, 2019 at 07:51:31PM +0200, Jernej Skrabec wrote:
> > +	dev->regmap = devm_regmap_init_mmio(dev->dev, dev->base,
> > +					    
&deinterlace_regmap_config);
> > +	if (IS_ERR(dev->regmap)) {
> > +		dev_err(dev->dev, "Couldn't create deinterlace 
regmap\n");
> > +
> > +		return PTR_ERR(dev->regmap);
> > +	}
> > +
> > +	ret = clk_prepare_enable(dev->bus_clk);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to enable bus clock\n");
> > +
> > +		return ret;
> > +	}
> 
> Do you need to keep the bus clock enabled all the time? Usually, for
> the SoCs that have a reset line, you only need it to read / write to
> the registers, not to have the controller actually running.
> 
> If you don't, then regmap_init_mmio_clk will take care of that for
> you.

I'll test that.

> 
> > +	clk_set_rate(dev->mod_clk, 300000000);
> > +
> > +	ret = clk_prepare_enable(dev->mod_clk);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to enable mod clock\n");
> > +
> > +		goto err_bus_clk;
> > +	}
> > +
> > +	ret = clk_prepare_enable(dev->ram_clk);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to enable ram clock\n");
> > +
> > +		goto err_mod_clk;
> > +	}
> > +
> > +	ret = reset_control_reset(dev->rstc);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to apply reset\n");
> > +
> > +		goto err_ram_clk;
> > +	}
> 
> This could be moved to a runtime_pm hook, with get_sync called in the
> open. That way you won't leave the device powered on if it's unused.

Ok.

> 
> > +struct deinterlace_dev {
> > +	struct v4l2_device	v4l2_dev;
> > +	struct video_device	vfd;
> > +	struct device		*dev;
> > +	struct v4l2_m2m_dev	*m2m_dev;
> > +
> > +	/* Device file mutex */
> > +	struct mutex		dev_mutex;
> > +
> > +	void __iomem		*base;
> > +	struct regmap		*regmap;
> 
> Do you need to store the base address in that structure if you're
> using the regmap?

Probably not. I'll remove it in v2.

Best regards,
Jernej

> 
> Maxime
Jernej Skrabec Sept. 12, 2019, 8:46 p.m. UTC | #6
Dne četrtek, 12. september 2019 ob 22:34:27 CEST je Maxime Ripard napisal(a):
> On Thu, Sep 12, 2019 at 10:28:37PM +0200, Jernej Škrabec wrote:
> > Dne četrtek, 12. september 2019 ob 22:20:57 CEST je Maxime Ripard 
napisal(a):
> > > Hi,
> > > 
> > > On Thu, Sep 12, 2019 at 07:51:29PM +0200, Jernej Skrabec wrote:
> > > > Both, H3 and H5, contain MBUS, which is the bus used by DMA devices to
> > > > access system memory.
> > > > 
> > > > MBUS controller is responsible for arbitration between channels based
> > > > on set priority and can do some other things as well, like report
> > > > bandwidth used. It also maps RAM region to different address than CPU.
> > > > 
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > ---
> > > > 
> > > >  arch/arm/boot/dts/sunxi-h3-h5.dtsi | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > > b/arch/arm/boot/dts/sunxi-h3-h5.dtsi index eba190b3f9de..ef1d03812636
> > > > 100644
> > > > --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > > +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > > @@ -109,6 +109,7 @@
> > > > 
> > > >  		compatible = "simple-bus";
> > > >  		#address-cells = <1>;
> > > >  		#size-cells = <1>;
> > > > 
> > > > +		dma-ranges;
> > > > 
> > > >  		ranges;
> > > >  		
> > > >  		display_clocks: clock@1000000 {
> > > > 
> > > > @@ -538,6 +539,14 @@
> > > > 
> > > >  			};
> > > >  		
> > > >  		};
> > > > 
> > > > +		mbus: dram-controller@1c62000 {
> > > > +			compatible = "allwinner,sun8i-h3-mbus";
> > > > +			reg = <0x01c62000 0x1000>;
> > > > +			clocks = <&ccu 113>;
> > > > +			dma-ranges = <0x00000000 0x40000000
> > 
> > 0xc0000000>;
> > 
> > > > +			#interconnect-cells = <1>;
> > > > +		};
> > > > +
> > > 
> > > If that's easy enough to access, can you also add the references in
> > > the devices that are already there? (CSI and DE comes to my mind, but
> > > there might be others).
> > 
> > Strangely, DE2 doesn't use this offset. That was tested on OrangePi
> > Plus2E,
> > which has 2 GiB of RAM and subtracting this offset causes corrupted image.
> 
> Ok, weird. But if it was tested then fine by me :)
> 
> > But I can add this properties to CSI too. However, wouldn't that need CSI
> > DT binding expansion with those properties? othetwise DT check will fail.
> Oh right, we definitely need to update the binding indeed. The code
> should be able to cope with both cases already.

I guess it's better to handle that with another patch series then? Changing 
CSI bindings doesn't fit here.

Best regards,
Jernej
Maxime Ripard Sept. 13, 2019, 7:57 a.m. UTC | #7
On Thu, Sep 12, 2019 at 10:46:58PM +0200, Jernej Škrabec wrote:
> Dne četrtek, 12. september 2019 ob 22:34:27 CEST je Maxime Ripard napisal(a):
> > On Thu, Sep 12, 2019 at 10:28:37PM +0200, Jernej Škrabec wrote:
> > > Dne četrtek, 12. september 2019 ob 22:20:57 CEST je Maxime Ripard 
> napisal(a):
> > > > Hi,
> > > > 
> > > > On Thu, Sep 12, 2019 at 07:51:29PM +0200, Jernej Skrabec wrote:
> > > > > Both, H3 and H5, contain MBUS, which is the bus used by DMA devices to
> > > > > access system memory.
> > > > > 
> > > > > MBUS controller is responsible for arbitration between channels based
> > > > > on set priority and can do some other things as well, like report
> > > > > bandwidth used. It also maps RAM region to different address than CPU.
> > > > > 
> > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > > ---
> > > > > 
> > > > >  arch/arm/boot/dts/sunxi-h3-h5.dtsi | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > > > b/arch/arm/boot/dts/sunxi-h3-h5.dtsi index eba190b3f9de..ef1d03812636
> > > > > 100644
> > > > > --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > > > +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > > > @@ -109,6 +109,7 @@
> > > > > 
> > > > >  		compatible = "simple-bus";
> > > > >  		#address-cells = <1>;
> > > > >  		#size-cells = <1>;
> > > > > 
> > > > > +		dma-ranges;
> > > > > 
> > > > >  		ranges;
> > > > >  		
> > > > >  		display_clocks: clock@1000000 {
> > > > > 
> > > > > @@ -538,6 +539,14 @@
> > > > > 
> > > > >  			};
> > > > >  		
> > > > >  		};
> > > > > 
> > > > > +		mbus: dram-controller@1c62000 {
> > > > > +			compatible = "allwinner,sun8i-h3-mbus";
> > > > > +			reg = <0x01c62000 0x1000>;
> > > > > +			clocks = <&ccu 113>;
> > > > > +			dma-ranges = <0x00000000 0x40000000
> > > 
> > > 0xc0000000>;
> > > 
> > > > > +			#interconnect-cells = <1>;
> > > > > +		};
> > > > > +
> > > > 
> > > > If that's easy enough to access, can you also add the references in
> > > > the devices that are already there? (CSI and DE comes to my mind, but
> > > > there might be others).
> > > 
> > > Strangely, DE2 doesn't use this offset. That was tested on OrangePi
> > > Plus2E,
> > > which has 2 GiB of RAM and subtracting this offset causes corrupted image.
> > 
> > Ok, weird. But if it was tested then fine by me :)
> > 
> > > But I can add this properties to CSI too. However, wouldn't that need CSI
> > > DT binding expansion with those properties? othetwise DT check will fail.
> > Oh right, we definitely need to update the binding indeed. The code
> > should be able to cope with both cases already.
> 
> I guess it's better to handle that with another patch series then? Changing 
> CSI bindings doesn't fit here.

Yeah, you can do it in a separate series if you prefer

Maxime
Maxime Ripard Sept. 13, 2019, 9:11 a.m. UTC | #8
Hi,

On Thu, Sep 12, 2019 at 10:43:28PM +0200, Jernej Škrabec wrote:
> Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a):
> > > +	clk_set_rate(dev->mod_clk, 300000000);

I just realized I missed this too. If you really need the rate to be
fixed, and if the controller cannot operate properly at any other
frequency, you probably want to use clk_set_rate_exclusive there.

Maxime
Jernej Skrabec Sept. 13, 2019, 8:06 p.m. UTC | #9
Hi!

Dne petek, 13. september 2019 ob 11:11:47 CEST je Maxime Ripard napisal(a):
> Hi,
> 
> On Thu, Sep 12, 2019 at 10:43:28PM +0200, Jernej Škrabec wrote:
> > Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard 
napisal(a):
> > > > +	clk_set_rate(dev->mod_clk, 300000000);
> 
> I just realized I missed this too. If you really need the rate to be
> fixed, and if the controller cannot operate properly at any other
> frequency, you probably want to use clk_set_rate_exclusive there.

I don't think that's needed. Parents of deinterlace clock are pll-periph0 and 
pll-periph1 which both have fixed clock and thus deinterlace clock will never 
be changed. I just set it to same frequency as it is set in BSP driver. I 
think it works with 600 MHz too, but that's overkill.

Best regards,
Jernej
Jernej Skrabec Sept. 14, 2019, 6:42 a.m. UTC | #10
Hi!

Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a):
> Hi,
> 
> On Thu, Sep 12, 2019 at 07:51:31PM +0200, Jernej Skrabec wrote:
> > +	dev->regmap = devm_regmap_init_mmio(dev->dev, dev->base,
> > +					    
&deinterlace_regmap_config);
> > +	if (IS_ERR(dev->regmap)) {
> > +		dev_err(dev->dev, "Couldn't create deinterlace 
regmap\n");
> > +
> > +		return PTR_ERR(dev->regmap);
> > +	}
> > +
> > +	ret = clk_prepare_enable(dev->bus_clk);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to enable bus clock\n");
> > +
> > +		return ret;
> > +	}
> 
> Do you need to keep the bus clock enabled all the time? Usually, for
> the SoCs that have a reset line, you only need it to read / write to
> the registers, not to have the controller actually running.
> 
> If you don't, then regmap_init_mmio_clk will take care of that for
> you.
> 
> > +	clk_set_rate(dev->mod_clk, 300000000);
> > +
> > +	ret = clk_prepare_enable(dev->mod_clk);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to enable mod clock\n");
> > +
> > +		goto err_bus_clk;
> > +	}
> > +
> > +	ret = clk_prepare_enable(dev->ram_clk);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to enable ram clock\n");
> > +
> > +		goto err_mod_clk;
> > +	}
> > +
> > +	ret = reset_control_reset(dev->rstc);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to apply reset\n");
> > +
> > +		goto err_ram_clk;
> > +	}
> 
> This could be moved to a runtime_pm hook, with get_sync called in the
> open. That way you won't leave the device powered on if it's unused.

Currently I'm looking at sun4i_csi.c as an example of runtime ops, but it 
seems a bit wrong to have suspend and resume function marked with 
__maybe_unused because they are the only functions which enable needed clocks.
If CONFIG_PM is not enabled, then this driver simply won't work, because 
clocks will never get enabled. I guess I can implement runtime pm ops in the 
same way and add additional handling when CONFIG_PM is not enabled, right?

BTW, which callback is get_sync? I don't see it in dev_pm_ops. I suppose I 
need only runtime_suspend and runtime_resume.

Off topic: sun6i_csi.c includes linux/pm_runtime.h but it doesn't have any kind 
of power management as far as I can see.

Best regards,
Jernej

> 
> > +struct deinterlace_dev {
> > +	struct v4l2_device	v4l2_dev;
> > +	struct video_device	vfd;
> > +	struct device		*dev;
> > +	struct v4l2_m2m_dev	*m2m_dev;
> > +
> > +	/* Device file mutex */
> > +	struct mutex		dev_mutex;
> > +
> > +	void __iomem		*base;
> > +	struct regmap		*regmap;
> 
> Do you need to store the base address in that structure if you're
> using the regmap?
> 
> Maxime
Maxime Ripard Sept. 18, 2019, 2:30 p.m. UTC | #11
Hi,

On Sat, Sep 14, 2019 at 08:42:22AM +0200, Jernej Škrabec wrote:
> Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a):
> > Hi,
> >
> > On Thu, Sep 12, 2019 at 07:51:31PM +0200, Jernej Skrabec wrote:
> > > +	dev->regmap = devm_regmap_init_mmio(dev->dev, dev->base,
> > > +
> &deinterlace_regmap_config);
> > > +	if (IS_ERR(dev->regmap)) {
> > > +		dev_err(dev->dev, "Couldn't create deinterlace
> regmap\n");
> > > +
> > > +		return PTR_ERR(dev->regmap);
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(dev->bus_clk);
> > > +	if (ret) {
> > > +		dev_err(dev->dev, "Failed to enable bus clock\n");
> > > +
> > > +		return ret;
> > > +	}
> >
> > Do you need to keep the bus clock enabled all the time? Usually, for
> > the SoCs that have a reset line, you only need it to read / write to
> > the registers, not to have the controller actually running.
> >
> > If you don't, then regmap_init_mmio_clk will take care of that for
> > you.
> >
> > > +	clk_set_rate(dev->mod_clk, 300000000);
> > > +
> > > +	ret = clk_prepare_enable(dev->mod_clk);
> > > +	if (ret) {
> > > +		dev_err(dev->dev, "Failed to enable mod clock\n");
> > > +
> > > +		goto err_bus_clk;
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(dev->ram_clk);
> > > +	if (ret) {
> > > +		dev_err(dev->dev, "Failed to enable ram clock\n");
> > > +
> > > +		goto err_mod_clk;
> > > +	}
> > > +
> > > +	ret = reset_control_reset(dev->rstc);
> > > +	if (ret) {
> > > +		dev_err(dev->dev, "Failed to apply reset\n");
> > > +
> > > +		goto err_ram_clk;
> > > +	}
> >
> > This could be moved to a runtime_pm hook, with get_sync called in the
> > open. That way you won't leave the device powered on if it's unused.
>
> Currently I'm looking at sun4i_csi.c as an example of runtime ops, but it
> seems a bit wrong to have suspend and resume function marked with
> __maybe_unused because they are the only functions which enable needed clocks.
> If CONFIG_PM is not enabled, then this driver simply won't work, because
> clocks will never get enabled. I guess I can implement runtime pm ops in the
> same way and add additional handling when CONFIG_PM is not enabled, right?

Ah, right. I guess you can either add a depends on PM, or you can call
the function directly and use set_active like we're doing in the SPI
driver.

> BTW, which callback is get_sync? I don't see it in dev_pm_ops. I suppose I
> need only runtime_suspend and runtime_resume.

get_sync is the user facing API, ie what you call when you want the
device to be powered up. This will call runtime_resume if needed
(there were no users, and you become the first one), and on the parent
devices if needed too (even though it's not our case).

> Off topic: sun6i_csi.c includes linux/pm_runtime.h but it doesn't have any kind
> of power management as far as I can see.

That's probably something we can remove then

Thanks!
Maxime
Jernej Skrabec Sept. 29, 2019, 1:18 p.m. UTC | #12
Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a):
> Hi,
> 
> On Thu, Sep 12, 2019 at 07:51:31PM +0200, Jernej Skrabec wrote:
> > +	dev->regmap = devm_regmap_init_mmio(dev->dev, dev->base,
> > +					    
&deinterlace_regmap_config);
> > +	if (IS_ERR(dev->regmap)) {
> > +		dev_err(dev->dev, "Couldn't create deinterlace 
regmap\n");
> > +
> > +		return PTR_ERR(dev->regmap);
> > +	}
> > +
> > +	ret = clk_prepare_enable(dev->bus_clk);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to enable bus clock\n");
> > +
> > +		return ret;
> > +	}
> 
> Do you need to keep the bus clock enabled all the time? Usually, for
> the SoCs that have a reset line, you only need it to read / write to
> the registers, not to have the controller actually running.
> 
> If you don't, then regmap_init_mmio_clk will take care of that for
> you.

I just tested and using regmap_init_mmio_clk() with "bus" clock doesn't work. 
I guess it has to be enabled whole time. I'll just leave it as-is.

Best regards,
Jernej

> 
> > +	clk_set_rate(dev->mod_clk, 300000000);
> > +
> > +	ret = clk_prepare_enable(dev->mod_clk);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to enable mod clock\n");
> > +
> > +		goto err_bus_clk;
> > +	}
> > +
> > +	ret = clk_prepare_enable(dev->ram_clk);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to enable ram clock\n");
> > +
> > +		goto err_mod_clk;
> > +	}
> > +
> > +	ret = reset_control_reset(dev->rstc);
> > +	if (ret) {
> > +		dev_err(dev->dev, "Failed to apply reset\n");
> > +
> > +		goto err_ram_clk;
> > +	}
> 
> This could be moved to a runtime_pm hook, with get_sync called in the
> open. That way you won't leave the device powered on if it's unused.
> 
> > +struct deinterlace_dev {
> > +	struct v4l2_device	v4l2_dev;
> > +	struct video_device	vfd;
> > +	struct device		*dev;
> > +	struct v4l2_m2m_dev	*m2m_dev;
> > +
> > +	/* Device file mutex */
> > +	struct mutex		dev_mutex;
> > +
> > +	void __iomem		*base;
> > +	struct regmap		*regmap;
> 
> Do you need to store the base address in that structure if you're
> using the regmap?
> 
> Maxime