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 <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> 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.
regards,
Peter.
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 <[email protected]>
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > 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. 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? So in theory we can
configure each devices' slice where ever we feel is appropriate? How
is memory allocated to the DMU and Audio drivers? Do you have scripts
which link the aforementioned binaries?
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.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
> > > Signed-off-by: Lee Jones <[email protected]>
> > > ---
> > > 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.
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.
Regards,
Loic
> -----Original Message-----
> From: Peter Griffin [mailto:[email protected]]
> Sent: Wednesday, March 16, 2016 9:11 PM
> To: Lee Jones <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-arm-
> [email protected]
> 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 <[email protected]>
> > > > Signed-off-by: Lee Jones <[email protected]>
> > > > ---
> > > > 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
> [email protected]
> http://www.stlinux.com/mailman/listinfo/kernel
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:[email protected]]
> > Sent: Wednesday, March 16, 2016 9:11 PM
> > To: Lee Jones <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; linux-arm-
> > [email protected]
> > 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 <[email protected]>
> > > > > Signed-off-by: Lee Jones <[email protected]>
> > > > > ---
> > > > > 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
> > [email protected]
> > 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