[v4,2/7] media: s5p-mfc: use generic reserved memory bindings
diff mbox

Message ID 1464096690-23605-3-git-send-email-m.szyprowski@samsung.com
State Not Applicable
Headers show

Commit Message

Marek Szyprowski May 24, 2016, 1:31 p.m. UTC
Use generic reserved memory bindings and mark old, custom properties
as obsoleted.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 .../devicetree/bindings/media/s5p-mfc.txt          | 39 +++++++++++++++++-----
 1 file changed, 31 insertions(+), 8 deletions(-)

Comments

Javier Martinez Canillas May 25, 2016, 3:18 p.m. UTC | #1
Hello Marek,

On 05/24/2016 09:31 AM, Marek Szyprowski wrote:
> Use generic reserved memory bindings and mark old, custom properties
> as obsoleted.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  .../devicetree/bindings/media/s5p-mfc.txt          | 39 +++++++++++++++++-----
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> index 2d5787e..92c94f5 100644
> --- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
> +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> @@ -21,15 +21,18 @@ Required properties:
>    - clock-names : from common clock binding: must contain "mfc",
>  		  corresponding to entry in the clocks property.
>  
> -  - samsung,mfc-r : Base address of the first memory bank used by MFC
> -		    for DMA contiguous memory allocation and its size.
> -
> -  - samsung,mfc-l : Base address of the second memory bank used by MFC
> -		    for DMA contiguous memory allocation and its size.
> -
>  Optional properties:
>    - power-domains : power-domain property defined with a phandle
>  			   to respective power domain.
> +  - memory-region : from reserved memory binding: phandles to two reserved
> +	memory regions, first is for "left" mfc memory bus interfaces,
> +	second if for the "right" mfc memory bus, used when no SYSMMU
> +	support is available
> +
> +Obsolete properties:
> +  - samsung,mfc-r, samsung,mfc-l : support removed, please use memory-region
> +	property instead
> +
> 

I wonder if we should maintain backward compatibility for this driver
since s5p-mfc memory allocation won't work with an old FDT if support
for the old properties are removed.

Although I'm not a big fan of keeping backward compatibility just to
add dead code that will never be used and I don't know of any Exynos
machine where the DTB and kernel are not updated together so I agree
with this patch:

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Rob Herring May 25, 2016, 5:36 p.m. UTC | #2
On Wed, May 25, 2016 at 11:18:59AM -0400, Javier Martinez Canillas wrote:
> Hello Marek,
> 
> On 05/24/2016 09:31 AM, Marek Szyprowski wrote:
> > Use generic reserved memory bindings and mark old, custom properties
> > as obsoleted.
> > 
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  .../devicetree/bindings/media/s5p-mfc.txt          | 39 +++++++++++++++++-----
> >  1 file changed, 31 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > index 2d5787e..92c94f5 100644
> > --- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > @@ -21,15 +21,18 @@ Required properties:
> >    - clock-names : from common clock binding: must contain "mfc",
> >  		  corresponding to entry in the clocks property.
> >  
> > -  - samsung,mfc-r : Base address of the first memory bank used by MFC
> > -		    for DMA contiguous memory allocation and its size.
> > -
> > -  - samsung,mfc-l : Base address of the second memory bank used by MFC
> > -		    for DMA contiguous memory allocation and its size.
> > -
> >  Optional properties:
> >    - power-domains : power-domain property defined with a phandle
> >  			   to respective power domain.
> > +  - memory-region : from reserved memory binding: phandles to two reserved
> > +	memory regions, first is for "left" mfc memory bus interfaces,
> > +	second if for the "right" mfc memory bus, used when no SYSMMU
> > +	support is available
> > +
> > +Obsolete properties:
> > +  - samsung,mfc-r, samsung,mfc-l : support removed, please use memory-region
> > +	property instead
> > +
> > 
> 
> I wonder if we should maintain backward compatibility for this driver
> since s5p-mfc memory allocation won't work with an old FDT if support
> for the old properties are removed.

Well, minimally the commit log should indicate that compatibility is 
being broken.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring May 25, 2016, 5:39 p.m. UTC | #3
On Tue, May 24, 2016 at 03:31:25PM +0200, Marek Szyprowski wrote:
> Use generic reserved memory bindings and mark old, custom properties
> as obsoleted.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  .../devicetree/bindings/media/s5p-mfc.txt          | 39 +++++++++++++++++-----
>  1 file changed, 31 insertions(+), 8 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski May 27, 2016, 6:19 a.m. UTC | #4
On 05/24/2016 03:31 PM, Marek Szyprowski wrote:
> Use generic reserved memory bindings and mark old, custom properties
> as obsoleted.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  .../devicetree/bindings/media/s5p-mfc.txt          | 39 +++++++++++++++++-----
>  1 file changed, 31 insertions(+), 8 deletions(-)
>

Acked-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski May 27, 2016, 6:37 a.m. UTC | #5
Hello,


On 2016-05-25 19:36, Rob Herring wrote:
> On Wed, May 25, 2016 at 11:18:59AM -0400, Javier Martinez Canillas wrote:
>> Hello Marek,
>>
>> On 05/24/2016 09:31 AM, Marek Szyprowski wrote:
>>> Use generic reserved memory bindings and mark old, custom properties
>>> as obsoleted.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   .../devicetree/bindings/media/s5p-mfc.txt          | 39 +++++++++++++++++-----
>>>   1 file changed, 31 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>> index 2d5787e..92c94f5 100644
>>> --- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>> +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>> @@ -21,15 +21,18 @@ Required properties:
>>>     - clock-names : from common clock binding: must contain "mfc",
>>>   		  corresponding to entry in the clocks property.
>>>   
>>> -  - samsung,mfc-r : Base address of the first memory bank used by MFC
>>> -		    for DMA contiguous memory allocation and its size.
>>> -
>>> -  - samsung,mfc-l : Base address of the second memory bank used by MFC
>>> -		    for DMA contiguous memory allocation and its size.
>>> -
>>>   Optional properties:
>>>     - power-domains : power-domain property defined with a phandle
>>>   			   to respective power domain.
>>> +  - memory-region : from reserved memory binding: phandles to two reserved
>>> +	memory regions, first is for "left" mfc memory bus interfaces,
>>> +	second if for the "right" mfc memory bus, used when no SYSMMU
>>> +	support is available
>>> +
>>> +Obsolete properties:
>>> +  - samsung,mfc-r, samsung,mfc-l : support removed, please use memory-region
>>> +	property instead
>>> +
>>>
>> I wonder if we should maintain backward compatibility for this driver
>> since s5p-mfc memory allocation won't work with an old FDT if support
>> for the old properties are removed.
> Well, minimally the commit log should indicate that compatibility is
> being broken.

Compatibility is only partially broken. I add this to the commit 
message. Old
bindings will still work with the new driver when IOMMU is enabled - in 
such case reserved
memory regions are ignored so this should not be a big issue. Using 
IOMMU also increases
total memory space for the video buffers without wasting it as 
'reserved'. Hope that
once those patches are merged, the IOMMU can be finally enabled in the 
exynos_defconfig.

Best regards
Javier Martinez Canillas May 27, 2016, 8:41 p.m. UTC | #6
Hello Marek,

On 05/27/2016 02:37 AM, Marek Szyprowski wrote:
> Hello,
> 
> 
> On 2016-05-25 19:36, Rob Herring wrote:
>> On Wed, May 25, 2016 at 11:18:59AM -0400, Javier Martinez Canillas wrote:
>>> Hello Marek,
>>>
>>> On 05/24/2016 09:31 AM, Marek Szyprowski wrote:
>>>> Use generic reserved memory bindings and mark old, custom properties
>>>> as obsoleted.
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>   .../devicetree/bindings/media/s5p-mfc.txt          | 39 +++++++++++++++++-----
>>>>   1 file changed, 31 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>>> index 2d5787e..92c94f5 100644
>>>> --- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>>> +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>>> @@ -21,15 +21,18 @@ Required properties:
>>>>     - clock-names : from common clock binding: must contain "mfc",
>>>>             corresponding to entry in the clocks property.
>>>>   -  - samsung,mfc-r : Base address of the first memory bank used by MFC
>>>> -            for DMA contiguous memory allocation and its size.
>>>> -
>>>> -  - samsung,mfc-l : Base address of the second memory bank used by MFC
>>>> -            for DMA contiguous memory allocation and its size.
>>>> -
>>>>   Optional properties:
>>>>     - power-domains : power-domain property defined with a phandle
>>>>                  to respective power domain.
>>>> +  - memory-region : from reserved memory binding: phandles to two reserved
>>>> +    memory regions, first is for "left" mfc memory bus interfaces,
>>>> +    second if for the "right" mfc memory bus, used when no SYSMMU
>>>> +    support is available
>>>> +
>>>> +Obsolete properties:
>>>> +  - samsung,mfc-r, samsung,mfc-l : support removed, please use memory-region
>>>> +    property instead
>>>> +
>>>>
>>> I wonder if we should maintain backward compatibility for this driver
>>> since s5p-mfc memory allocation won't work with an old FDT if support
>>> for the old properties are removed.
>> Well, minimally the commit log should indicate that compatibility is
>> being broken.
> 
> Compatibility is only partially broken. I add this to the commit message. Old
> bindings will still work with the new driver when IOMMU is enabled - in such case reserved
> memory regions are ignored so this should not be a big issue. Using IOMMU also increases
> total memory space for the video buffers without wasting it as 'reserved'. Hope that
> once those patches are merged, the IOMMU can be finally enabled in the exynos_defconfig.
>

Yes, a problem is that some Exynos machines (for example the Snow and Peach
Chromebooks) fail to boot when IOMMU is enabled due the bootloader leaving
the FIMD enabled doing DMA operations automatically as you found before.

You proposed to add a "iommu-reserved-mapping" property [0] but that never
landed due not having an agreement on how should be fixed properly IIUC [1].

So that has to be fixed before enabling the Exynos IOMMU support by default.

[0]: http://www.spinics.net/lists/arm-kernel/msg415501.html
[1]: http://www.spinics.net/lists/arm-kernel/msg419747.html 

Best regards,

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
index 2d5787e..92c94f5 100644
--- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
+++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
@@ -21,15 +21,18 @@  Required properties:
   - clock-names : from common clock binding: must contain "mfc",
 		  corresponding to entry in the clocks property.
 
-  - samsung,mfc-r : Base address of the first memory bank used by MFC
-		    for DMA contiguous memory allocation and its size.
-
-  - samsung,mfc-l : Base address of the second memory bank used by MFC
-		    for DMA contiguous memory allocation and its size.
-
 Optional properties:
   - power-domains : power-domain property defined with a phandle
 			   to respective power domain.
+  - memory-region : from reserved memory binding: phandles to two reserved
+	memory regions, first is for "left" mfc memory bus interfaces,
+	second if for the "right" mfc memory bus, used when no SYSMMU
+	support is available
+
+Obsolete properties:
+  - samsung,mfc-r, samsung,mfc-l : support removed, please use memory-region
+	property instead
+
 
 Example:
 SoC specific DT entry:
@@ -43,9 +46,29 @@  mfc: codec@13400000 {
 	clock-names = "mfc";
 };
 
+Reserved memory specific DT entry for given board (see reserved memory binding
+for more information):
+
+reserved-memory {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges;
+
+	mfc_left: region@51000000 {
+		compatible = "shared-dma-pool";
+		no-map;
+		reg = <0x51000000 0x800000>;
+	};
+
+	mfc_right: region@43000000 {
+		compatible = "shared-dma-pool";
+		no-map;
+		reg = <0x43000000 0x800000>;
+	};
+};
+
 Board specific DT entry:
 
 codec@13400000 {
-	samsung,mfc-r = <0x43000000 0x800000>;
-	samsung,mfc-l = <0x51000000 0x800000>;
+	memory-region = <&mfc_left>, <&mfc_right>;
 };