Message-ID: <60e95a1a76241653be0b3a2da0c4d017@mail.ncrmnt.org>
Date:	Wed, 01 Jul 2015 00:33:13 +0300
From:	Andrew <andrew@...mnt.org>
To:	Laura Abbott <labbott@...hat.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devicetree@...r.kernel.org, pebolle@...cali.nl,
	Sudip Mukherjee <sudipm.mukherjee@...il.com>,
	Arve Hj�nnev�g <arve@...roid.com>,
	Riley Andrews <riandrews@...roid.com>,
	Chen Gang <gang.chen.5i5j@...il.com>,
	Fabian Frederick <fabf@...net.be>,
	Android Kernel Team <kernel-team@...roid.com>,
	linux-kernel@...r.kernel.org, devel@...uxdriverproject.org,
	john.stultz@...aro.org
Subject: Re: [PATCH v5.1 2/2] staging: ion: Add ion-physmem documentation

Laura Abbott писал 30.06.2015 20:54:
> (adding devicetree mailing list since I didn't see it cc-ed)
> 
> Please also remember to cc the staging list since Ion is
> still a staging framework.
> 
> On 06/30/2015 08:34 AM, Andrew Andrianov wrote:
>> Signed-off-by: Andrew Andrianov <andrew@...mnt.org>
>> ---
>>   Documentation/devicetree/bindings/ion,physmem.txt | 98 
>> +++++++++++++++++++++++
> 
> The proper place is bindings/staging/ since Ion is still a staging 
> driver

Got it, will fix and send for the next respin

> 
>>   1 file changed, 98 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/ion,physmem.txt 
>> b/Documentation/devicetree/bindings/ion,physmem.txt
>> new file mode 100644
>> index 0000000..e8c64dd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ion,physmem.txt
>> @@ -0,0 +1,98 @@
>> +ION PhysMem Driver
>> +#include <dt-bindings/ion,physmem.h>
>> +
>> +
>> +ION PhysMem is a generic driver for ION Memory Manager that allows 
>> you to
>> +define ION Memory Manager heaps using device tree. This is mostly 
>> useful if
>> +your SoC has several 'special' regions (e.g. SRAM, dedicated memory 
>> banks,
>> +etc) that are present in the physical memory map and you want to add 
>> them to
>> +ION as heaps of memory.
>> +
> 
> A good start of a description. This could use a bit more detail about 
> what the
> Ion memory framework actually does (i.e. tied really strongly to 
> Android)

Ironically we use ION without android. We even started of a liblinuxion
for use in traditional linux userspace (should be up at RC Module's 
github pretty soon)
I'll add a bit more words here, that's not a problem.

> 
> You are also missing a generic description of what all goes into the 
> binding.
> Based on what you have below you would get
> 
> (name) : ion@(base-address) {
> compatible = "ion,physmem";
> reg = <(baseaddr) (size)>;
> reg-names = "memory";
> ion-heap-id = <(int-value)>;
> ion-heap-type = <(int-value)>;
> ion-heap-align = <(int-value)>;
> ion-heap-name = "(string value");
> };
> 
> and then you need to describe what each of those properties actually 
> do.
> Having all the examples is still really useful, especially for heaps 
> such as the
> system heaps which are independent of any memory map.

Got it, will fix.

>> +
>> +Examples:
>> +
>> +1. 256KiB On-chip SRAM used as ION DMA heap. reg range is treated as 
>> a physical
>> +   address range
>> +
>> +	ion_im0: ion@...0100000 {
>> +	     compatible = "ion,physmem";
>> +	     reg = <0x00100000 0x40000>;
>> +	     reg-names = "memory";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "IM0";
>> +	};
>> +
>> +2. The same, but using system DMA memory.
>> +
>> +	ion_dma: ion@...eadbeef {
>> +	     compatible = "ion,physmem";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "SYSDMA";
>> +	};
> 
> CMA now has bindings upstream. I'd rather see Ion be a CMA client 
> instead
> of creating any other bindings.

Unfortunately, this breaks the most useful case for us, when ion uses
several dedicated physical memory areas. Maybe wrap CMA and use it as
another ion-heap-type then?

> 
>> +
>> +3. Carveout heap, 1MiB size, ion-physmem will alloc pages for it 
>> using
>> +   alloc_pages_exact(). reg range is used for specifying size only.
>> +
>> +		ion_crv: ion@...dbeef {
>> +		     compatible = "ion,physmem";
>> +		     reg = <0x00000000 0x100000>;
>> +		     reg-names = "memory";
>> +		     ion-heap-id   = <3>;
>> +		     ion-heap-type = <ION_HEAP_TYPE_CARVEOUT>;
>> +		     ion-heap-align = <0x10>;
>> +		     ion-heap-name = "carveout";
>> +		};
>> +
> 
> My understanding of DT binding rules was that for foo@X, your reg must
> equal X. Maintainers can correct me if I'm wrong or if that restriction
> is relaxed these days.

In case reg doesn't represent a physical memory region, but only size 
here
(for convenient resource_size calls) we may end with several ion@0 this 
way.
Is it really required to be so?

> 
>> +4. Chunk heap. 1MiB size, ion-physmem will alloc pages for it using
>> +   alloc_pages_exact(). reg range is used for specifying size only.
>> +
>> +	ion_chunk: ion@...eadbeef {
>> +	     compatible = "ion,physmem";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_CHUNK>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "chunky";
>> +	};
>> +
>> +
>> +5. vmalloc();
>> +
>> +	ion_chunk: ion@...eadbeef {
>> +	     compatible = "ion,physmem";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "sys";
>> +	};
>> +
>> +6. kmalloc();
>> +
>> +	ion_chunk: ion@...eadbeef {
>> +	     compatible = "ion,physmem";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_SYSTEM_CONTIG>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "syscont";
>> +	};
>> +
>> +If the underlying heap relies on some physical device that needs 
>> clock
>> +gating, you may need to fill the clocks field in. E.g.:
>> +
>> +
>> +	ion_im0: ion@...0100000 {
>> +	     compatible = "ion,physmem";
>> +	     reg = <0x00100000 0x40000>;
>> +	     reg-names = "memory";
>> +	     ion-heap-id   = <2>;
>> +	     ion-heap-type = <ION_HEAP_TYPE_DMA>;
>> +	     ion-heap-align = <0x10>;
>> +	     ion-heap-name = "IM0";
>> +	     clocks = <&oscillator_27m>;
>> +	     clock-names = "clk_27m";
>> +	};
>> +
>> +ion-physmem will do everything required to enable and disable the 
>> clock.
>> 
> 
> I'm glad to see an attempt to get bindings submitted for Ion. There
> exists other bindings out of tree already[1]. My one concern here is 
> that
> Ion is so 'experimental/staging' that trying to
> shoot for an ABI is going to be difficult because of how far this has 
> to
> go. On the other hand, it's been out there long enough and it's in use
> so establishing something for what there is at the present would be
> beneficial. I also don't know the rules on DT bindings for staging 
> drivers
> so I'll let the maintainers comment.

So far ION looks like the only proper way for our weird use case and I'm 
strictly
against reinventing the wheel and yet another allocator for all our DSP 
needs as long
as ION gets the job done.

> 
> Thanks,
> Laura
> 
> 
> [1]
> https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/Documentation/devicetree/bindings/arm/msm/msm_ion.txt?h=msm-3.10


-- 
Regards,
Andrew
RC Module :: http://module.ru
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists