[RFC,V1,1/6] dt-bindings: mt8183: Add binding for DIP shared memory
diff mbox series

Message ID 20190417104511.21514-2-frederic.chen@mediatek.com
State Changes Requested
Headers show
Series
  • media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Frederic Chen April 17, 2019, 10:45 a.m. UTC
This patch adds the binding for describing the shared memory
used to exchange configuration and tuning data between the
co-processor and Digital Image Processing (DIP) unit of the
camera ISP system on Mediatek SoCs.

Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
---
 .../mediatek,reserve-memory-dip_smem.txt      | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt

Comments

Rob Herring April 30, 2019, 1:15 a.m. UTC | #1
On Wed, Apr 17, 2019 at 06:45:06PM +0800, Frederic Chen wrote:
> This patch adds the binding for describing the shared memory
> used to exchange configuration and tuning data between the
> co-processor and Digital Image Processing (DIP) unit of the
> camera ISP system on Mediatek SoCs.
> 
> Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> ---
>  .../mediatek,reserve-memory-dip_smem.txt      | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> new file mode 100644
> index 000000000000..64c001b476b9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> @@ -0,0 +1,45 @@
> +Mediatek DIP Shared Memory binding
> +
> +This binding describes the shared memory, which serves the purpose of
> +describing the shared memory region used to exchange data between Digital
> +Image Processing (DIP) and co-processor in Mediatek SoCs.
> +
> +The co-processor doesn't have the iommu so we need to use the physical
> +address to access the shared buffer in the firmware.
> +
> +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> +it can use dma address to access the memory region.
> +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
> +
> +
> +Required properties:
> +
> +- compatible: must be "mediatek,reserve-memory-dip_smem"

Don't use '_'.

> +
> +- reg: required for static allocation (see reserved-memory.txt for
> +  the detailed usage)
> +
> +- alloc-range: required for dynamic allocation. The range must
> +  between 0x00000400 and 0x100000000 due to the co-processer's
> +  addressing limitation

Generally, you should pick either static or dynamic allocation for a 
given binding. Static if there's some address restriction or sharing, 
dynamic if not.

Sounds like static in this case.

> +
> +- size: required for dynamic allocation. The unit is bytes.
> +  If you want to enable the full feature of Digital Processing Unit,
> +  you need 20 MB at least.
> +
> +
> +Example:
> +
> +The following example shows the DIP shared memory setup for MT8183.
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +		reserve-memory-dip_smem {
> +			compatible = "mediatek,reserve-memory-dip_smem";
> +			size = <0 0x1400000>;
> +			alignment = <0 0x1000>;
> +			alloc-ranges = <0 0x40000000 0 0x50000000>;
> +		};
> +	};
> -- 
> 2.18.0
>
Frederic Chen May 7, 2019, 2:22 p.m. UTC | #2
Dear Rob,

I appreciate your comments.

On Mon, 2019-04-29 at 20:15 -0500, Rob Herring wrote:
> On Wed, Apr 17, 2019 at 06:45:06PM +0800, Frederic Chen wrote:
> > This patch adds the binding for describing the shared memory
> > used to exchange configuration and tuning data between the
> > co-processor and Digital Image Processing (DIP) unit of the
> > camera ISP system on Mediatek SoCs.
> > 
> > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > ---
> >  .../mediatek,reserve-memory-dip_smem.txt      | 45 +++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > new file mode 100644
> > index 000000000000..64c001b476b9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > @@ -0,0 +1,45 @@
> > +Mediatek DIP Shared Memory binding
> > +
> > +This binding describes the shared memory, which serves the purpose of
> > +describing the shared memory region used to exchange data between Digital
> > +Image Processing (DIP) and co-processor in Mediatek SoCs.
> > +
> > +The co-processor doesn't have the iommu so we need to use the physical
> > +address to access the shared buffer in the firmware.
> > +
> > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> > +it can use dma address to access the memory region.
> > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
> > +
> > +
> > +Required properties:
> > +
> > +- compatible: must be "mediatek,reserve-memory-dip_smem"
> 
> Don't use '_'.

I got it. I will use "mediatek,reserve-memory-dip-smem" instead in next
version of the patch

> 
> > +
> > +- reg: required for static allocation (see reserved-memory.txt for
> > +  the detailed usage)
> > +
> > +- alloc-range: required for dynamic allocation. The range must
> > +  between 0x00000400 and 0x100000000 due to the co-processer's
> > +  addressing limitation
> 
> Generally, you should pick either static or dynamic allocation for a 
> given binding. Static if there's some address restriction or sharing, 
> dynamic if not.
> 
> Sounds like static in this case.
> 

DIP reserved memory has address restriction so it is the static case. I
would like to remove the dynamic allocation part and modify the
description as following:

- reg: required for DIP. The range must be between 0x00000400 and
  0x100000000 due to the co-processor's addressing limitation.
  The size must be 26MB. Please see reserved-memory.txt for the 
  detailed usage.

> > +
> > +- size: required for dynamic allocation. The unit is bytes.
> > +  If you want to enable the full feature of Digital Processing Unit,
> > +  you need 20 MB at least.
> > +
> > +
> > +Example:
> > +
> > +The following example shows the DIP shared memory setup for MT8183.
> > +
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +		reserve-memory-dip_smem {
> > +			compatible = "mediatek,reserve-memory-dip_smem";
> > +			size = <0 0x1400000>;
> > +			alignment = <0 0x1000>;
> > +			alloc-ranges = <0 0x40000000 0 0x50000000>;
> > +		};
> > +	};
> > -- 
> > 2.18.0
> > 

Sincerely,

Frederic Chen
Rob Herring May 14, 2019, 4:19 p.m. UTC | #3
On Tue, May 7, 2019 at 9:22 AM Frederic Chen <frederic.chen@mediatek.com> wrote:
>
> Dear Rob,
>
> I appreciate your comments.
>
> On Mon, 2019-04-29 at 20:15 -0500, Rob Herring wrote:
> > On Wed, Apr 17, 2019 at 06:45:06PM +0800, Frederic Chen wrote:
> > > This patch adds the binding for describing the shared memory
> > > used to exchange configuration and tuning data between the
> > > co-processor and Digital Image Processing (DIP) unit of the
> > > camera ISP system on Mediatek SoCs.
> > >
> > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > ---
> > >  .../mediatek,reserve-memory-dip_smem.txt      | 45 +++++++++++++++++++
> > >  1 file changed, 45 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > new file mode 100644
> > > index 000000000000..64c001b476b9
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > @@ -0,0 +1,45 @@
> > > +Mediatek DIP Shared Memory binding
> > > +
> > > +This binding describes the shared memory, which serves the purpose of
> > > +describing the shared memory region used to exchange data between Digital
> > > +Image Processing (DIP) and co-processor in Mediatek SoCs.
> > > +
> > > +The co-processor doesn't have the iommu so we need to use the physical
> > > +address to access the shared buffer in the firmware.
> > > +
> > > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> > > +it can use dma address to access the memory region.
> > > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
> > > +
> > > +
> > > +Required properties:
> > > +
> > > +- compatible: must be "mediatek,reserve-memory-dip_smem"
> >
> > Don't use '_'.
>
> I got it. I will use "mediatek,reserve-memory-dip-smem" instead in next
> version of the patch
>
> >
> > > +
> > > +- reg: required for static allocation (see reserved-memory.txt for
> > > +  the detailed usage)
> > > +
> > > +- alloc-range: required for dynamic allocation. The range must
> > > +  between 0x00000400 and 0x100000000 due to the co-processer's
> > > +  addressing limitation
> >
> > Generally, you should pick either static or dynamic allocation for a
> > given binding. Static if there's some address restriction or sharing,
> > dynamic if not.
> >
> > Sounds like static in this case.
> >
>
> DIP reserved memory has address restriction so it is the static case. I
> would like to remove the dynamic allocation part and modify the
> description as following:
>
> - reg: required for DIP. The range must be between 0x00000400 and
>   0x100000000 due to the co-processor's addressing limitation.
>   The size must be 26MB. Please see reserved-memory.txt for the
>   detailed usage.

You can use dma-ranges to define addressing translations and
restrictions like this. That will in turn set the device's dma-mask to
ensure allocations are done in a region that is addressable.

But if you have a known, fixed size, then a carve out with
reserved-memory is fine.

Rob
Tomasz Figa May 16, 2019, 6:12 a.m. UTC | #4
On Wed, May 15, 2019 at 1:19 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, May 7, 2019 at 9:22 AM Frederic Chen <frederic.chen@mediatek.com> wrote:
> >
> > Dear Rob,
> >
> > I appreciate your comments.
> >
> > On Mon, 2019-04-29 at 20:15 -0500, Rob Herring wrote:
> > > On Wed, Apr 17, 2019 at 06:45:06PM +0800, Frederic Chen wrote:
> > > > This patch adds the binding for describing the shared memory
> > > > used to exchange configuration and tuning data between the
> > > > co-processor and Digital Image Processing (DIP) unit of the
> > > > camera ISP system on Mediatek SoCs.
> > > >
> > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > > ---
> > > >  .../mediatek,reserve-memory-dip_smem.txt      | 45 +++++++++++++++++++
> > > >  1 file changed, 45 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > > new file mode 100644
> > > > index 000000000000..64c001b476b9
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > > @@ -0,0 +1,45 @@
> > > > +Mediatek DIP Shared Memory binding
> > > > +
> > > > +This binding describes the shared memory, which serves the purpose of
> > > > +describing the shared memory region used to exchange data between Digital
> > > > +Image Processing (DIP) and co-processor in Mediatek SoCs.
> > > > +
> > > > +The co-processor doesn't have the iommu so we need to use the physical
> > > > +address to access the shared buffer in the firmware.
> > > > +
> > > > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> > > > +it can use dma address to access the memory region.
> > > > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
> > > > +
> > > > +
> > > > +Required properties:
> > > > +
> > > > +- compatible: must be "mediatek,reserve-memory-dip_smem"
> > >
> > > Don't use '_'.
> >
> > I got it. I will use "mediatek,reserve-memory-dip-smem" instead in next
> > version of the patch
> >
> > >
> > > > +
> > > > +- reg: required for static allocation (see reserved-memory.txt for
> > > > +  the detailed usage)
> > > > +
> > > > +- alloc-range: required for dynamic allocation. The range must
> > > > +  between 0x00000400 and 0x100000000 due to the co-processer's
> > > > +  addressing limitation
> > >
> > > Generally, you should pick either static or dynamic allocation for a
> > > given binding. Static if there's some address restriction or sharing,
> > > dynamic if not.
> > >
> > > Sounds like static in this case.
> > >
> >
> > DIP reserved memory has address restriction so it is the static case. I
> > would like to remove the dynamic allocation part and modify the
> > description as following:
> >
> > - reg: required for DIP. The range must be between 0x00000400 and
> >   0x100000000 due to the co-processor's addressing limitation.
> >   The size must be 26MB. Please see reserved-memory.txt for the
> >   detailed usage.
>
> You can use dma-ranges to define addressing translations and
> restrictions like this. That will in turn set the device's dma-mask to
> ensure allocations are done in a region that is addressable.
>
> But if you have a known, fixed size, then a carve out with
> reserved-memory is fine.

There is also another aspect here. The coprocessor that we're
allocating the memory for is behind an MPU that must be programmed
completely in one go and locked for security reasons to ensure that
the coprocessor itself doesn't rewrite the MPU settings. That means
that we need to have all the allocations completed before booting that
coprocessor.

To be honest, I'd adopt a completely different design here.

We're going to have a driver for that coprocessor (SCP) and IMHO any
shared memory areas should belong to it. Then, any specific drivers
talking to the firmware running on SCP should ask the SCP driver to
allocate some memory from its fixed pool. WDYT?

Best regards,
Tomasz
Rob Herring May 17, 2019, 10:22 p.m. UTC | #5
On Thu, May 16, 2019 at 1:12 AM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Wed, May 15, 2019 at 1:19 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, May 7, 2019 at 9:22 AM Frederic Chen <frederic.chen@mediatek.com> wrote:
> > >
> > > Dear Rob,
> > >
> > > I appreciate your comments.
> > >
> > > On Mon, 2019-04-29 at 20:15 -0500, Rob Herring wrote:
> > > > On Wed, Apr 17, 2019 at 06:45:06PM +0800, Frederic Chen wrote:
> > > > > This patch adds the binding for describing the shared memory
> > > > > used to exchange configuration and tuning data between the
> > > > > co-processor and Digital Image Processing (DIP) unit of the
> > > > > camera ISP system on Mediatek SoCs.
> > > > >
> > > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > > > ---
> > > > >  .../mediatek,reserve-memory-dip_smem.txt      | 45 +++++++++++++++++++
> > > > >  1 file changed, 45 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > > > new file mode 100644
> > > > > index 000000000000..64c001b476b9
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > > > @@ -0,0 +1,45 @@
> > > > > +Mediatek DIP Shared Memory binding
> > > > > +
> > > > > +This binding describes the shared memory, which serves the purpose of
> > > > > +describing the shared memory region used to exchange data between Digital
> > > > > +Image Processing (DIP) and co-processor in Mediatek SoCs.
> > > > > +
> > > > > +The co-processor doesn't have the iommu so we need to use the physical
> > > > > +address to access the shared buffer in the firmware.
> > > > > +
> > > > > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> > > > > +it can use dma address to access the memory region.
> > > > > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
> > > > > +
> > > > > +
> > > > > +Required properties:
> > > > > +
> > > > > +- compatible: must be "mediatek,reserve-memory-dip_smem"
> > > >
> > > > Don't use '_'.
> > >
> > > I got it. I will use "mediatek,reserve-memory-dip-smem" instead in next
> > > version of the patch
> > >
> > > >
> > > > > +
> > > > > +- reg: required for static allocation (see reserved-memory.txt for
> > > > > +  the detailed usage)
> > > > > +
> > > > > +- alloc-range: required for dynamic allocation. The range must
> > > > > +  between 0x00000400 and 0x100000000 due to the co-processer's
> > > > > +  addressing limitation
> > > >
> > > > Generally, you should pick either static or dynamic allocation for a
> > > > given binding. Static if there's some address restriction or sharing,
> > > > dynamic if not.
> > > >
> > > > Sounds like static in this case.
> > > >
> > >
> > > DIP reserved memory has address restriction so it is the static case. I
> > > would like to remove the dynamic allocation part and modify the
> > > description as following:
> > >
> > > - reg: required for DIP. The range must be between 0x00000400 and
> > >   0x100000000 due to the co-processor's addressing limitation.
> > >   The size must be 26MB. Please see reserved-memory.txt for the
> > >   detailed usage.
> >
> > You can use dma-ranges to define addressing translations and
> > restrictions like this. That will in turn set the device's dma-mask to
> > ensure allocations are done in a region that is addressable.
> >
> > But if you have a known, fixed size, then a carve out with
> > reserved-memory is fine.
>
> There is also another aspect here. The coprocessor that we're
> allocating the memory for is behind an MPU that must be programmed
> completely in one go and locked for security reasons to ensure that
> the coprocessor itself doesn't rewrite the MPU settings. That means
> that we need to have all the allocations completed before booting that
> coprocessor.
>
> To be honest, I'd adopt a completely different design here.
>
> We're going to have a driver for that coprocessor (SCP) and IMHO any
> shared memory areas should belong to it. Then, any specific drivers
> talking to the firmware running on SCP should ask the SCP driver to
> allocate some memory from its fixed pool. WDYT?

That's more than just an address restriction, so yeah, use a
reserved-memory area.

Rob

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
new file mode 100644
index 000000000000..64c001b476b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
@@ -0,0 +1,45 @@ 
+Mediatek DIP Shared Memory binding
+
+This binding describes the shared memory, which serves the purpose of
+describing the shared memory region used to exchange data between Digital
+Image Processing (DIP) and co-processor in Mediatek SoCs.
+
+The co-processor doesn't have the iommu so we need to use the physical
+address to access the shared buffer in the firmware.
+
+The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
+it can use dma address to access the memory region.
+(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
+
+
+Required properties:
+
+- compatible: must be "mediatek,reserve-memory-dip_smem"
+
+- reg: required for static allocation (see reserved-memory.txt for
+  the detailed usage)
+
+- alloc-range: required for dynamic allocation. The range must
+  between 0x00000400 and 0x100000000 due to the co-processer's
+  addressing limitation
+
+- size: required for dynamic allocation. The unit is bytes.
+  If you want to enable the full feature of Digital Processing Unit,
+  you need 20 MB at least.
+
+
+Example:
+
+The following example shows the DIP shared memory setup for MT8183.
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		reserve-memory-dip_smem {
+			compatible = "mediatek,reserve-memory-dip_smem";
+			size = <0 0x1400000>;
+			alignment = <0 0x1000>;
+			alloc-ranges = <0 0x40000000 0 0x50000000>;
+		};
+	};