Message-ID: <20160317101828.GA13692@x1>
Date:	Thu, 17 Mar 2016 10:18:28 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	Loic PALLARDY <loic.pallardy@...com>
Cc:	Peter Griffin <peter.griffin@...aro.org>,
	"ohad@...ery.com" <ohad@...ery.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"f.fainelli@...il.com" <f.fainelli@...il.com>,
	"kernel@...inux.com" <kernel@...inux.com>,
	"Nathan_Lynch@...tor.com" <Nathan_Lynch@...tor.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"s-anna@...com" <s-anna@...com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [STLinux Kernel] [PATCH v5 7/7] ARM: STiH407: Move over to using
 the 'reserved-memory' API for obtaining DMA memory

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@...aro.org]
> > Sent: Wednesday, March 16, 2016 9:11 PM
> > To: Lee Jones <lee.jones@...aro.org>
> > Cc: ohad@...ery.com; devicetree@...r.kernel.org; f.fainelli@...il.com;
> > kernel@...inux.com; Nathan_Lynch@...tor.com; linux-
> > kernel@...r.kernel.org; s-anna@...com; linux-arm-
> > kernel@...ts.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@...com>
> > > > > Signed-off-by: Lee Jones <lee.jones@...aro.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@...00000 {
> > > > > +			compatible = "shared-dma-pool";
> > > > > +			reg = <0x40000000 0x01000000>;
> > > > > +			no-map;
> > > > > +		};
> > > > > +
> > > > > +		gp1_reserved: rproc@...00000 {
> > > > > +			compatible = "shared-dma-pool";
> > > > > +			reg = <0x41000000 0x01000000>;
> > > > > +			no-map;
> > > > > +		};
> > > > > +
> > > > > +		audio_reserved: rproc@...00000 {
> > > > > +			compatible = "shared-dma-pool";
> > > > > +			reg = <0x42000000 0x01000000>;
> > > > > +			no-map;
> > > > > +		};
> > > > > +
> > > > > +		dmu_reserved: rproc@...00000 {
> > > > > +			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@...inux.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

Powered by blists - more mailing lists