diff mbox

[v5,7/7] ARM: STiH407: Move over to using the 'reserved-memory' API for obtaining DMA memory

Message ID 1452602781-22424-8-git-send-email-lee.jones@linaro.org
State Superseded
Headers show

Commit Message

Lee Jones Jan. 12, 2016, 12:46 p.m. UTC
Doing so saves quite a bit of code in the driver.

For more information on the 'reserved-memory' bindings see:

  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt

Suggested-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>

---
 arch/arm/boot/dts/stih407-family.dtsi | 46 +++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 8 deletions(-)

-- 
1.9.1

Comments

Lee Jones March 17, 2016, 10:18 a.m. UTC | #1
On Thu, 17 Mar 2016, Loic PALLARDY wrote:

> Hi Lee, Pete,

> 

> The coprocessor memory map defined below is for test. Addresses have

> been arbitrary fixed.

> The audio and video firmware you want to use are for product

> configuration.

> For sure memory mapping must be adapted or firmware recompiled.

> 

> About coherency between firmware characteristics (linked address,

> position independent or not, size) and DT definition, you're right,

> some checks are missing in this version. It shouldn't be possible to

> load/start a firmware if DT reserved memory is not aligned with

> firmware resource table and firmware start address.

> 

> Lee, I propose to have a short discussion during next ST LT weekly

> meeting to list missing features and identify ST internal remoteproc

> patches for upstream.


Sounds good to me.

FYI: This is only the first patch-set submission, which only provides
basic support.  There are subsequent sets, which I will start to
refine after the merge-window closes.  Perhaps the internal patches
you speak of are already part of this set?

> > -----Original Message-----

> > From: Peter Griffin [mailto:peter.griffin@linaro.org]

> > Sent: Wednesday, March 16, 2016 9:11 PM

> > To: Lee Jones <lee.jones@linaro.org>

> > Cc: ohad@wizery.com; devicetree@vger.kernel.org; f.fainelli@gmail.com;

> > kernel@stlinux.com; Nathan_Lynch@mentor.com; linux-

> > kernel@vger.kernel.org; s-anna@ti.com; linux-arm-

> > kernel@lists.infradead.org

> > Subject: Re: [STLinux Kernel] [PATCH v5 7/7] ARM: STiH407: Move over to

> > using the 'reserved-memory' API for obtaining DMA memory

> > 

> > Hi Lee,

> > 

> > On Wed, 16 Mar 2016, Lee Jones wrote:

> > 

> > > On Wed, 16 Mar 2016, Peter Griffin wrote:

> > >

> > > > Hi Lee,

> > > >

> > > > On Tue, 12 Jan 2016, Lee Jones wrote:

> > > >

> > > > > Doing so saves quite a bit of code in the driver.

> > > > >

> > > > > For more information on the 'reserved-memory' bindings see:

> > > > >

> > > > >

> > > > > Documentation/devicetree/bindings/reserved-memory/reserved-

> > memory.

> > > > > txt

> > > > >

> > > > > Suggested-by: Suman Anna <s-anna@ti.com>

> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>

> > > > > ---

> > > > >  arch/arm/boot/dts/stih407-family.dtsi | 46

> > > > > +++++++++++++++++++++++++++++------

> > > > >  1 file changed, 38 insertions(+), 8 deletions(-)

> > > > >

> > > > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi

> > > > > b/arch/arm/boot/dts/stih407-family.dtsi

> > > > > index 15c20b6..27b8efc 100644

> > > > > --- a/arch/arm/boot/dts/stih407-family.dtsi

> > > > > +++ b/arch/arm/boot/dts/stih407-family.dtsi

> > > > > @@ -15,6 +15,36 @@

> > > > >  	#address-cells = <1>;

> > > > >  	#size-cells = <1>;

> > > > >

> > > > > +	reserved-memory {

> > > > > +		#address-cells = <1>;

> > > > > +		#size-cells = <1>;

> > > > > +		ranges;

> > > > > +

> > > > > +		gp0_reserved: rproc@40000000 {

> > > > > +			compatible = "shared-dma-pool";

> > > > > +			reg = <0x40000000 0x01000000>;

> > > > > +			no-map;

> > > > > +		};

> > > > > +

> > > > > +		gp1_reserved: rproc@41000000 {

> > > > > +			compatible = "shared-dma-pool";

> > > > > +			reg = <0x41000000 0x01000000>;

> > > > > +			no-map;

> > > > > +		};

> > > > > +

> > > > > +		audio_reserved: rproc@42000000 {

> > > > > +			compatible = "shared-dma-pool";

> > > > > +			reg = <0x42000000 0x01000000>;

> > > > > +			no-map;

> > > > > +		};

> > > > > +

> > > > > +		dmu_reserved: rproc@43000000 {

> > > > > +			compatible = "shared-dma-pool";

> > > > > +			reg = <0x43000000 0x01000000>;

> > > > > +			no-map;

> > > > > +		};

> > > >

> > > > I don't believe these reserved memory ranges are correct for

> > audio_reserved and dmu_reserved.

> > > >

> > > > For example my vid_firmware-stih407.elf is linked at 0x41c00000 base

> > > > address and my audio_firmware-bd-stih407.elf is linked at 0x40c00000.

> > > >

> > > > So with all the st231 rproc nodes enabled I guess it would still

> > > > work. But currently I think st231_gp0 is reserving the memory region

> > > > for st231_audio, and st231-gp1 is reserving the memory region for

> > st231_dmu.

> > >

> > > These addresses are taken from internally tested code.

> > 

> > Yes I did check the internal kernel, it would appear to be wrong there as well.

> > One of the joys of mailing list code review I guess :-)

> > 

> > > I don't have

> > > access to the LMI layout documentation (if it even exists) so can't

> > > check for myself.

> > 

> > >  Isn't this just DDR anyway?

> > 

> > Yes it is DDR

> > 

> > > So in theory we can

> > > configure each devices' slice where ever we feel is appropriate?

> > 

> > Nope. The st231 audio and video firmwares are provided by ST as binary

> > blobs and aren't AFAIK compiled as position independent code. So the

> > reserved-memory region needs to match where the firmware has been

> > linked to run from.

> > 

> > > How

> > > is memory allocated to the DMU and Audio drivers?  Do you have scripts

> > > which link the aforementioned binaries?

> > 

> > I don't have any scripts, firmware source code or even a st200 toolset.

> > 

> > >

> > > If you think there is an issue, I suggest the best thing to do is ping

> > > Ludovic, since he is the author of the original code.

> > 

> > Ok I will ping Ludovic and point him at this thread.

> > 

> > I think maybe the internal kernel rproc driver was only used to reserve

> > memory, manage clocks, and co-processor reset / power lines, and multicom

> > actually loaded the firmware elf file.

> > 

> > The reason for coming to that conclusion is that if rproc driver was loading the

> > firmware I can't see how you would end up with a correctly booted co-

> > processor with a reserved-memory node which doesn't match up with

> > where the firmware is linked to run from.

> > 

> > Did you manage to boot audio or video co-pro successfully with the dt nodes

> > as they currently are in this patch?

> > 

> > regards,

> > 

> > Peter.

> > 

> > _______________________________________________

> > Kernel mailing list

> > Kernel@stlinux.com

> > http://www.stlinux.com/mailman/listinfo/kernel


-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
diff mbox

Patch

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 15c20b6..27b8efc 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -15,6 +15,36 @@ 
 	#address-cells = <1>;
 	#size-cells = <1>;
 
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		gp0_reserved: rproc@40000000 {
+			compatible = "shared-dma-pool";
+			reg = <0x40000000 0x01000000>;
+			no-map;
+		};
+
+		gp1_reserved: rproc@41000000 {
+			compatible = "shared-dma-pool";
+			reg = <0x41000000 0x01000000>;
+			no-map;
+		};
+
+		audio_reserved: rproc@42000000 {
+			compatible = "shared-dma-pool";
+			reg = <0x42000000 0x01000000>;
+			no-map;
+		};
+
+		dmu_reserved: rproc@43000000 {
+			compatible = "shared-dma-pool";
+			reg = <0x43000000 0x01000000>;
+			no-map;
+		};
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
@@ -651,9 +681,9 @@ 
 			status		= "okay";
 		};
 
-		st231_gp0: st231-gp0@40000000 {
+		st231_gp0: st231-gp0 {
 			compatible	= "st,st231-rproc";
-			reg		= <0x40000000 0x01000000>;
+			memory-region	= <&gp0_reserved>;
 			resets		= <&softreset STIH407_ST231_GP0_SOFTRESET>;
 			reset-names	= "sw_reset";
 			clocks		= <&clk_s_c0_flexgen CLK_ST231_GP_0>;
@@ -661,9 +691,9 @@ 
 			st,syscfg	= <&syscfg_core 0x22c>;
 		};
 
-		st231_gp1: st231-gp1@41000000 {
+		st231_gp1: st231-gp1 {
 			compatible	= "st,st231-rproc";
-			reg		= <0x41000000 0x01000000>;
+			memory-region	= <&gp1_reserved>;
 			resets		= <&softreset STIH407_ST231_GP1_SOFTRESET>;
 			reset-names	= "sw_reset";
 			clocks		= <&clk_s_c0_flexgen CLK_ST231_GP_1>;
@@ -671,9 +701,9 @@ 
 			st,syscfg	= <&syscfg_core 0x220>;
 		};
 
-		st231_audio: st231-audio@42000000 {
+		st231_audio: st231-audio {
 			compatible	= "st,st231-rproc";
-			reg		= <0x42000000 0x01000000>;
+			memory-region	= <&audio_reserved>;
 			resets		= <&softreset STIH407_ST231_AUD_SOFTRESET>;
 			reset-names	= "sw_reset";
 			clocks		= <&clk_s_c0_flexgen CLK_ST231_AUD_0>;
@@ -681,9 +711,9 @@ 
 			st,syscfg	= <&syscfg_core 0x228>;
 		};
 
-		st231_dmu: st231-dmu@43000000 {
+		st231_dmu: st231-dmu {
 			compatible	= "st,st231-rproc";
-			reg		= <0x43000000 0x01000000>;
+			memory-region	= <&dmu_reserved>;
 			resets		= <&softreset STIH407_ST231_DMU_SOFTRESET>;
 			reset-names	= "sw_reset";
 			clocks		= <&clk_s_c0_flexgen CLK_ST231_DMU>;