[00/17] remoteproc: omap changes on top of v5.4-rc1

Message ID 20191028124238.19224-1-t-kristo@ti.com
Headers show
Series
  • remoteproc: omap changes on top of v5.4-rc1
Related show

Message

Tero Kristo Oct. 28, 2019, 12:42 p.m.
Hi,

Mostly this is a facelift of the pretty old OMAP remoteproc code base,
supporting now DT, runtime PM, ti-sysc, and dropping legacy omap hwmod
support. Also timer + watchdog support is added, and couple of bugfixes.

The series hasn't been posted before mostly due to missing OMAP reset
support, which has just been lately posted out and is getting merged to
5.5.

For proper functionality, this series depends at least on the clock [1]
and reset [2] series sent out by me, but to compile, it only needs [1].

-Tero

[1] TI Clock series: https://patchwork.kernel.org/cover/11143025/
[2] TI Reset series: https://patchwork.kernel.org/cover/11179573/


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Comments

Rob Herring Nov. 6, 2019, 3:27 a.m. | #1
On Mon, Oct 28, 2019 at 02:42:22PM +0200, Tero Kristo wrote:
> From: Suman Anna <s-anna@ti.com>

> 

> Add the device tree bindings document for the IPU and DSP

> remote processor devices on OMAP4+ SoCs.

> 

> Cc: Rob Herring <robh@kernel.org>

> Cc: devicetree@vger.kernel.org

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

> Signed-off-by: Tero Kristo <t-kristo@ti.com>

> ---

>  .../remoteproc/ti,omap-remoteproc.txt         | 205 ++++++++++++++++++

>  1 file changed, 205 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt

>


Looks to be in pretty good shape, but how about doing a schema.

> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt b/Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt

> new file mode 100644

> index 000000000000..e2bcfcab21c1

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt

> @@ -0,0 +1,205 @@

> +OMAP4+ Remoteproc Devices

> +=========================

> +

> +The OMAP family of SoCs usually have one or more slave processor sub-systems

> +that are used to offload some of the processor-intensive tasks, or to manage

> +other hardware accelerators, for achieving various system level goals.

> +

> +The processor cores in the sub-system are usually behind an IOMMU, and may

> +contain additional sub-modules like Internal RAM and/or ROMs, L1 and/or L2

> +caches, an Interrupt Controller, a Cache Controller etc.

> +

> +The OMAP SoCs usually have a DSP processor sub-system and/or an IPU processor

> +sub-system. The DSP processor sub-system can contain any of the TI's C64x,

> +C66x or C67x family of DSP cores as the main execution unit. The IPU processor

> +sub-system usually contains either a Dual-Core Cortex-M3 or Dual-Core Cortex-M4

> +processors.

> +

> +Remote Processor Node:

> +======================

> +Each remote processor sub-system is represented as a single DT node. Each node

> +has a number of required or optional properties that enable the OS running on

> +the host processor (MPU) to perform the device management of the remote

> +processor and to communicate with the remote processor. The various properties

> +can be classified as constant or variable. The constant properties are dictated

> +by the SoC and does not change from one board to another having the same SoC.

> +Examples of constant properties include 'iommus', 'reg'. The variable properties

> +are dictated by the system integration aspects such as memory on the board, or

> +configuration used within the corresponding firmware image. Examples of variable

> +properties include 'mboxes', 'memory-region', 'timers', 'watchdog-timers' etc.

> +

> +Required properties:

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

> +The following are the mandatory properties:

> +

> +- compatible:	Should be one of the following,

> +		    "ti,omap4-dsp" for DSPs on OMAP4 SoCs

> +		    "ti,omap5-dsp" for DSPs on OMAP5 SoCs

> +		    "ti,dra7-dsp" for DSPs on DRA7xx/AM57xx SoCs

> +		    "ti,omap4-ipu" for IPUs on OMAP4 SoCs

> +		    "ti,omap5-ipu" for IPUs on OMAP5 SoCs

> +		    "ti,dra7-ipu" for IPUs on DRA7xx/AM57xx SoCs

> +

> +- iommus:	phandles to OMAP IOMMU nodes, that need to be programmed

> +		for this remote processor to access any external RAM memory or

> +		other peripheral device address spaces. This property usually

> +		has only a single phandle. Multiple phandles are used only in

> +		cases where the sub-system has different ports for different

> +		sub-modules within the processor sub-system (eg: DRA7 DSPs),

> +		and need the same programming in both the MMUs.

> +

> +- mboxes:	OMAP Mailbox specifier denoting the sub-mailbox, to be used for

> +		communication with the remote processor. The specifier format is

> +		as per the bindings,

> +		Documentation/devicetree/bindings/mailbox/omap-mailbox.txt

> +		This property should match with the sub-mailbox node used in

> +		the firmware image.

> +

> +Optional properties:

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

> +Some of these properties are mandatory on some SoCs, and some are optional

> +depending on the configuration of the firmware image to be executed on the

> +remote processor. The conditions are mentioned for each property.

> +

> +The following are the optional properties:

> +- reg:			Address space for any remoteproc memories present on

> +			the SoC. Should contain an entry for each value in

> +			'reg-names'. These are mandatory for all DSP and IPU

> +			processors that have them (OMAP4/OMAP5 DSPs do not have

> +			any RAMs)

> +

> +- reg-names:		Required names for each of the address spaces defined in

> +			the 'reg' property. Should contain a string from among

> +			the following names, each representing the corresponding

> +			internal RAM memory region,

> +			   "l2ram" for L2 RAM,

> +			   "l1pram" for L1 Program RAM Memory/Cache,

> +			   "l1dram" for L1 Data RAM Memory/Cache,

> +

> +			All devices may not have all the above memories.

> +

> +- syscon-bootreg:	Should be a pair of the phandle to the System Control


ti,bootreg

> +			Configuration region that contains the boot address

> +			register, and the register offset of the boot address

> +			register within the System Control module. This property

> +			is required for all the DSP instances on OMAP4, OMAP5

> +			and DRA7xx SoCs.

> +

> +- memory-region:	phandle to the reserved memory node to be associated

> +			with the remoteproc device. The reserved memory node

> +			can be a CMA memory node, and should be defined as

> +			per the bindings,

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

> +

> +- mbox-names:		Optional names for the OMAP mailbox specifiers mentioned

> +			in the 'mboxes' property, one per specifier value


From the mboxes description, seemed like only one entry?

Need to define the values here.

> +

> +- timers:		One or more phandles to OMAP DMTimer nodes, that serve

> +			as System/Tick timers for the OS running on the remote

> +			processors. This will usually be a single timer if the

> +			processor sub-system is running in SMP mode, or one per

> +			core in the processor sub-system. This can also be used

> +			to reserve specific timers to be dedicated to the

> +			remote processors.

> +

> +			This property is mandatory on remote processors requiring

> +			external tick wakeup, and to support Power Management

> +			features. The timers to be used should match with the

> +			timers used in the firmware image.

> +

> +- watchdog-timers:	One or more phandles to OMAP DMTimer nodes, used to

> +			serve as Watchdog timers for the processor cores. This

> +			will usually be one per executing processor core, even

> +			if the processor sub-system is running a SMP OS.

> +

> +			The timers to be used should match with the watchdog

> +			timers used in the firmware image.


These 2 are not standard names. Either need 'ti,' prefix or we should 
standardize them. There's been some discussion of an input capture 
binding and I was wondering if it should be more general to any 
timer function.

> +

> +Example:

> +--------

> +

> +1. OMAP4 DSP

> +	/* DSP Reserved Memory node */

> +	reserved-memory {

> +		#address-cells = <1>;

> +		#size-cells = <1>;

> +		ranges;

> +

> +		dsp_memory_region: dsp-memory@98000000 {

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

> +			reg = <0x98000000 0x800000>;

> +			reusable;

> +		};

> +	};

> +

> +	/* DSP node */

> +	ocp {

> +		dsp: dsp {

> +			compatible = "ti,omap4-dsp";

> +			syscon-bootreg = <&scm_conf 0x304>;

> +			iommus = <&mmu_dsp>;

> +			mboxes = <&mailbox &mbox_dsp>;

> +			memory-region = <&dsp_memory_region>;

> +			timers = <&timer5>;

> +			watchdog-timers = <&timer6>;

> +		};

> +	};

> +

> +2. OMAP5 IPU

> +	/* IPU Reserved Memory node */

> +	reserved-memory {

> +		#address-cells = <2>;

> +		#size-cells = <2>;

> +		ranges;

> +

> +		ipu_memory_region: ipu-memory@95800000 {

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

> +			reg = <0 0x95800000 0 0x3800000>;

> +			reusable;

> +		};

> +	};

> +

> +	/* IPU node */

> +	ocp {

> +		ipu: ipu@55020000 {

> +			compatible = "ti,omap5-ipu";

> +			reg = <0x55020000 0x10000>;

> +			reg-names = "l2ram";

> +			iommus = <&mmu_ipu>;

> +			mboxes = <&mailbox &mbox_ipu>;

> +			memory-region = <&ipu_memory_region>;

> +			timers = <&timer3>, <&timer4>;

> +			watchdog-timers = <&timer9>, <&timer11>;

> +		};

> +	};

> +

> +3. DRA7xx/AM57xx DSP

> +	/* DSP1 Reserved Memory node */

> +	reserved-memory {

> +		#address-cells = <2>;

> +		#size-cells = <2>;

> +		ranges;

> +

> +		dsp1_memory_region: dsp1-memory@99000000 {

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

> +			reg = <0x0 0x99000000 0x0 0x4000000>;

> +			reusable;

> +		};

> +	};

> +

> +	/* DSP1 node */

> +	ocp {

> +		dsp1: dsp@40800000 {

> +			compatible = "ti,dra7-dsp";

> +			reg = <0x40800000 0x48000>,

> +			      <0x40e00000 0x8000>,

> +			      <0x40f00000 0x8000>;

> +			reg-names = "l2ram", "l1pram", "l1dram";

> +			syscon-bootreg = <&scm_conf 0x55c>;

> +			iommus = <&mmu0_dsp1>, <&mmu1_dsp1>;

> +			mboxes = <&mailbox5 &mbox_dsp1_ipc3x>;

> +			memory-region = <&dsp1_memory_region>;

> +			timers = <&timer5>;

> +			watchdog-timers = <&timer10>;

> +		};

> +	};

> -- 

> 2.17.1

> 

> --

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Tero Kristo Nov. 6, 2019, 12:44 p.m. | #2
On 06/11/2019 05:27, Rob Herring wrote:
> On Mon, Oct 28, 2019 at 02:42:22PM +0200, Tero Kristo wrote:

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

>>

>> Add the device tree bindings document for the IPU and DSP

>> remote processor devices on OMAP4+ SoCs.

>>

>> Cc: Rob Herring <robh@kernel.org>

>> Cc: devicetree@vger.kernel.org

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

>> Signed-off-by: Tero Kristo <t-kristo@ti.com>

>> ---

>>   .../remoteproc/ti,omap-remoteproc.txt         | 205 ++++++++++++++++++

>>   1 file changed, 205 insertions(+)

>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt

>>

> 

> Looks to be in pretty good shape, but how about doing a schema.


iommu / mailbox is not in schema format, can I just convert this one to 
schema without considering those? If yes, I can go ahead and do it.

> 

>> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt b/Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt

>> new file mode 100644

>> index 000000000000..e2bcfcab21c1

>> --- /dev/null

>> +++ b/Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt

>> @@ -0,0 +1,205 @@

>> +OMAP4+ Remoteproc Devices

>> +=========================

>> +

>> +The OMAP family of SoCs usually have one or more slave processor sub-systems

>> +that are used to offload some of the processor-intensive tasks, or to manage

>> +other hardware accelerators, for achieving various system level goals.

>> +

>> +The processor cores in the sub-system are usually behind an IOMMU, and may

>> +contain additional sub-modules like Internal RAM and/or ROMs, L1 and/or L2

>> +caches, an Interrupt Controller, a Cache Controller etc.

>> +

>> +The OMAP SoCs usually have a DSP processor sub-system and/or an IPU processor

>> +sub-system. The DSP processor sub-system can contain any of the TI's C64x,

>> +C66x or C67x family of DSP cores as the main execution unit. The IPU processor

>> +sub-system usually contains either a Dual-Core Cortex-M3 or Dual-Core Cortex-M4

>> +processors.

>> +

>> +Remote Processor Node:

>> +======================

>> +Each remote processor sub-system is represented as a single DT node. Each node

>> +has a number of required or optional properties that enable the OS running on

>> +the host processor (MPU) to perform the device management of the remote

>> +processor and to communicate with the remote processor. The various properties

>> +can be classified as constant or variable. The constant properties are dictated

>> +by the SoC and does not change from one board to another having the same SoC.

>> +Examples of constant properties include 'iommus', 'reg'. The variable properties

>> +are dictated by the system integration aspects such as memory on the board, or

>> +configuration used within the corresponding firmware image. Examples of variable

>> +properties include 'mboxes', 'memory-region', 'timers', 'watchdog-timers' etc.

>> +

>> +Required properties:

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

>> +The following are the mandatory properties:

>> +

>> +- compatible:	Should be one of the following,

>> +		    "ti,omap4-dsp" for DSPs on OMAP4 SoCs

>> +		    "ti,omap5-dsp" for DSPs on OMAP5 SoCs

>> +		    "ti,dra7-dsp" for DSPs on DRA7xx/AM57xx SoCs

>> +		    "ti,omap4-ipu" for IPUs on OMAP4 SoCs

>> +		    "ti,omap5-ipu" for IPUs on OMAP5 SoCs

>> +		    "ti,dra7-ipu" for IPUs on DRA7xx/AM57xx SoCs

>> +

>> +- iommus:	phandles to OMAP IOMMU nodes, that need to be programmed

>> +		for this remote processor to access any external RAM memory or

>> +		other peripheral device address spaces. This property usually

>> +		has only a single phandle. Multiple phandles are used only in

>> +		cases where the sub-system has different ports for different

>> +		sub-modules within the processor sub-system (eg: DRA7 DSPs),

>> +		and need the same programming in both the MMUs.


^ the target of this is not in schema.

>> +

>> +- mboxes:	OMAP Mailbox specifier denoting the sub-mailbox, to be used for

>> +		communication with the remote processor. The specifier format is

>> +		as per the bindings,

>> +		Documentation/devicetree/bindings/mailbox/omap-mailbox.txt

>> +		This property should match with the sub-mailbox node used in

>> +		the firmware image.


^ Neither this one.

>> +

>> +Optional properties:

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

>> +Some of these properties are mandatory on some SoCs, and some are optional

>> +depending on the configuration of the firmware image to be executed on the

>> +remote processor. The conditions are mentioned for each property.

>> +

>> +The following are the optional properties:

>> +- reg:			Address space for any remoteproc memories present on

>> +			the SoC. Should contain an entry for each value in

>> +			'reg-names'. These are mandatory for all DSP and IPU

>> +			processors that have them (OMAP4/OMAP5 DSPs do not have

>> +			any RAMs)

>> +

>> +- reg-names:		Required names for each of the address spaces defined in

>> +			the 'reg' property. Should contain a string from among

>> +			the following names, each representing the corresponding

>> +			internal RAM memory region,

>> +			   "l2ram" for L2 RAM,

>> +			   "l1pram" for L1 Program RAM Memory/Cache,

>> +			   "l1dram" for L1 Data RAM Memory/Cache,

>> +

>> +			All devices may not have all the above memories.

>> +

>> +- syscon-bootreg:	Should be a pair of the phandle to the System Control

> 

> ti,bootreg


This one I can fix.

> 

>> +			Configuration region that contains the boot address

>> +			register, and the register offset of the boot address

>> +			register within the System Control module. This property

>> +			is required for all the DSP instances on OMAP4, OMAP5

>> +			and DRA7xx SoCs.

>> +

>> +- memory-region:	phandle to the reserved memory node to be associated

>> +			with the remoteproc device. The reserved memory node

>> +			can be a CMA memory node, and should be defined as

>> +			per the bindings,

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

>> +

>> +- mbox-names:		Optional names for the OMAP mailbox specifiers mentioned

>> +			in the 'mboxes' property, one per specifier value

> 

>  From the mboxes description, seemed like only one entry?

> 

> Need to define the values here.


I think I can just ditch this. The current driver doesn't care about the 
name at all. It is not used in any of the examples / current DT data either.

> 

>> +

>> +- timers:		One or more phandles to OMAP DMTimer nodes, that serve

>> +			as System/Tick timers for the OS running on the remote

>> +			processors. This will usually be a single timer if the

>> +			processor sub-system is running in SMP mode, or one per

>> +			core in the processor sub-system. This can also be used

>> +			to reserve specific timers to be dedicated to the

>> +			remote processors.

>> +

>> +			This property is mandatory on remote processors requiring

>> +			external tick wakeup, and to support Power Management

>> +			features. The timers to be used should match with the

>> +			timers used in the firmware image.

>> +

>> +- watchdog-timers:	One or more phandles to OMAP DMTimer nodes, used to

>> +			serve as Watchdog timers for the processor cores. This

>> +			will usually be one per executing processor core, even

>> +			if the processor sub-system is running a SMP OS.

>> +

>> +			The timers to be used should match with the watchdog

>> +			timers used in the firmware image.

> 

> These 2 are not standard names. Either need 'ti,' prefix or we should

> standardize them. There's been some discussion of an input capture

> binding and I was wondering if it should be more general to any

> timer function.


I'll convert these to ti,xyz for now.

-Tero

> 

>> +

>> +Example:

>> +--------

>> +

>> +1. OMAP4 DSP

>> +	/* DSP Reserved Memory node */

>> +	reserved-memory {

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

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

>> +		ranges;

>> +

>> +		dsp_memory_region: dsp-memory@98000000 {

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

>> +			reg = <0x98000000 0x800000>;

>> +			reusable;

>> +		};

>> +	};

>> +

>> +	/* DSP node */

>> +	ocp {

>> +		dsp: dsp {

>> +			compatible = "ti,omap4-dsp";

>> +			syscon-bootreg = <&scm_conf 0x304>;

>> +			iommus = <&mmu_dsp>;

>> +			mboxes = <&mailbox &mbox_dsp>;

>> +			memory-region = <&dsp_memory_region>;

>> +			timers = <&timer5>;

>> +			watchdog-timers = <&timer6>;

>> +		};

>> +	};

>> +

>> +2. OMAP5 IPU

>> +	/* IPU Reserved Memory node */

>> +	reserved-memory {

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

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

>> +		ranges;

>> +

>> +		ipu_memory_region: ipu-memory@95800000 {

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

>> +			reg = <0 0x95800000 0 0x3800000>;

>> +			reusable;

>> +		};

>> +	};

>> +

>> +	/* IPU node */

>> +	ocp {

>> +		ipu: ipu@55020000 {

>> +			compatible = "ti,omap5-ipu";

>> +			reg = <0x55020000 0x10000>;

>> +			reg-names = "l2ram";

>> +			iommus = <&mmu_ipu>;

>> +			mboxes = <&mailbox &mbox_ipu>;

>> +			memory-region = <&ipu_memory_region>;

>> +			timers = <&timer3>, <&timer4>;

>> +			watchdog-timers = <&timer9>, <&timer11>;

>> +		};

>> +	};

>> +

>> +3. DRA7xx/AM57xx DSP

>> +	/* DSP1 Reserved Memory node */

>> +	reserved-memory {

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

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

>> +		ranges;

>> +

>> +		dsp1_memory_region: dsp1-memory@99000000 {

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

>> +			reg = <0x0 0x99000000 0x0 0x4000000>;

>> +			reusable;

>> +		};

>> +	};

>> +

>> +	/* DSP1 node */

>> +	ocp {

>> +		dsp1: dsp@40800000 {

>> +			compatible = "ti,dra7-dsp";

>> +			reg = <0x40800000 0x48000>,

>> +			      <0x40e00000 0x8000>,

>> +			      <0x40f00000 0x8000>;

>> +			reg-names = "l2ram", "l1pram", "l1dram";

>> +			syscon-bootreg = <&scm_conf 0x55c>;

>> +			iommus = <&mmu0_dsp1>, <&mmu1_dsp1>;

>> +			mboxes = <&mailbox5 &mbox_dsp1_ipc3x>;

>> +			memory-region = <&dsp1_memory_region>;

>> +			timers = <&timer5>;

>> +			watchdog-timers = <&timer10>;

>> +		};

>> +	};

>> -- 

>> 2.17.1

>>

>> --


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Rob Herring Nov. 6, 2019, 1:27 p.m. | #3
On Wed, Nov 6, 2019 at 6:44 AM Tero Kristo <t-kristo@ti.com> wrote:
>

> On 06/11/2019 05:27, Rob Herring wrote:

> > On Mon, Oct 28, 2019 at 02:42:22PM +0200, Tero Kristo wrote:

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

> >>

> >> Add the device tree bindings document for the IPU and DSP

> >> remote processor devices on OMAP4+ SoCs.

> >>

> >> Cc: Rob Herring <robh@kernel.org>

> >> Cc: devicetree@vger.kernel.org

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

> >> Signed-off-by: Tero Kristo <t-kristo@ti.com>

> >> ---

> >>   .../remoteproc/ti,omap-remoteproc.txt         | 205 ++++++++++++++++++

> >>   1 file changed, 205 insertions(+)

> >>   create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt

> >>

> >

> > Looks to be in pretty good shape, but how about doing a schema.

>

> iommu / mailbox is not in schema format, can I just convert this one to

> schema without considering those? If yes, I can go ahead and do it.


The client side both have schema (in dt-schema repo).

> >> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt b/Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt

> >> new file mode 100644

> >> index 000000000000..e2bcfcab21c1

> >> --- /dev/null

> >> +++ b/Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.txt

> >> @@ -0,0 +1,205 @@

> >> +OMAP4+ Remoteproc Devices

> >> +=========================

> >> +

> >> +The OMAP family of SoCs usually have one or more slave processor sub-systems

> >> +that are used to offload some of the processor-intensive tasks, or to manage

> >> +other hardware accelerators, for achieving various system level goals.

> >> +

> >> +The processor cores in the sub-system are usually behind an IOMMU, and may

> >> +contain additional sub-modules like Internal RAM and/or ROMs, L1 and/or L2

> >> +caches, an Interrupt Controller, a Cache Controller etc.

> >> +

> >> +The OMAP SoCs usually have a DSP processor sub-system and/or an IPU processor

> >> +sub-system. The DSP processor sub-system can contain any of the TI's C64x,

> >> +C66x or C67x family of DSP cores as the main execution unit. The IPU processor

> >> +sub-system usually contains either a Dual-Core Cortex-M3 or Dual-Core Cortex-M4

> >> +processors.

> >> +

> >> +Remote Processor Node:

> >> +======================

> >> +Each remote processor sub-system is represented as a single DT node. Each node

> >> +has a number of required or optional properties that enable the OS running on

> >> +the host processor (MPU) to perform the device management of the remote

> >> +processor and to communicate with the remote processor. The various properties

> >> +can be classified as constant or variable. The constant properties are dictated

> >> +by the SoC and does not change from one board to another having the same SoC.

> >> +Examples of constant properties include 'iommus', 'reg'. The variable properties

> >> +are dictated by the system integration aspects such as memory on the board, or

> >> +configuration used within the corresponding firmware image. Examples of variable

> >> +properties include 'mboxes', 'memory-region', 'timers', 'watchdog-timers' etc.

> >> +

> >> +Required properties:

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

> >> +The following are the mandatory properties:

> >> +

> >> +- compatible:       Should be one of the following,

> >> +                "ti,omap4-dsp" for DSPs on OMAP4 SoCs

> >> +                "ti,omap5-dsp" for DSPs on OMAP5 SoCs

> >> +                "ti,dra7-dsp" for DSPs on DRA7xx/AM57xx SoCs

> >> +                "ti,omap4-ipu" for IPUs on OMAP4 SoCs

> >> +                "ti,omap5-ipu" for IPUs on OMAP5 SoCs

> >> +                "ti,dra7-ipu" for IPUs on DRA7xx/AM57xx SoCs

> >> +

> >> +- iommus:   phandles to OMAP IOMMU nodes, that need to be programmed

> >> +            for this remote processor to access any external RAM memory or

> >> +            other peripheral device address spaces. This property usually

> >> +            has only a single phandle. Multiple phandles are used only in

> >> +            cases where the sub-system has different ports for different

> >> +            sub-modules within the processor sub-system (eg: DRA7 DSPs),

> >> +            and need the same programming in both the MMUs.

>

> ^ the target of this is not in schema.


You mean the OMAP IOMMU binding? That doesn't matter at all.

Rob
Bjorn Andersson Nov. 11, 2019, 11:18 p.m. | #4
On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

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

> 

> The DSP remote processors on OMAP SoCs require a boot register to

> be programmed with a boot address, and this boot address needs to

> be on a 1KB boundary. The current code is simply masking the boot

> address appropriately without performing any sanity checks before

> releasing the resets. An unaligned boot address results in an

> undefined execution behavior and can result in various bus errors

> like MMU Faults or L3 NoC errors. Such errors are hard to debug and

> can be easily avoided by adding a sanity check for the alignment

> before booting a DSP remote processor.

> 

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

> Signed-off-by: Tero Kristo <t-kristo@ti.com>


Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


> ---

>  drivers/remoteproc/omap_remoteproc.c | 18 +++++++++++++++---

>  1 file changed, 15 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

> index cd776189d20b..a10377547533 100644

> --- a/drivers/remoteproc/omap_remoteproc.c

> +++ b/drivers/remoteproc/omap_remoteproc.c

> @@ -124,13 +124,22 @@ static void omap_rproc_kick(struct rproc *rproc, int vqid)

>   *

>   * Set boot address for a supported DSP remote processor.

>   */

> -static void omap_rproc_write_dsp_boot_addr(struct rproc *rproc)

> +static int omap_rproc_write_dsp_boot_addr(struct rproc *rproc)

>  {

> +	struct device *dev = rproc->dev.parent;

>  	struct omap_rproc *oproc = rproc->priv;

>  	struct omap_rproc_boot_data *bdata = oproc->boot_data;

>  	u32 offset = bdata->boot_reg;

>  

> +	if (rproc->bootaddr & (SZ_1K - 1)) {

> +		dev_err(dev, "invalid boot address 0x%x, must be aligned on a 1KB boundary\n",

> +			rproc->bootaddr);

> +		return -EINVAL;

> +	}

> +

>  	regmap_write(bdata->syscon, offset, rproc->bootaddr);

> +

> +	return 0;

>  }

>  

>  /*

> @@ -147,8 +156,11 @@ static int omap_rproc_start(struct rproc *rproc)

>  	int ret;

>  	struct mbox_client *client = &oproc->client;

>  

> -	if (oproc->boot_data)

> -		omap_rproc_write_dsp_boot_addr(rproc);

> +	if (oproc->boot_data) {

> +		ret = omap_rproc_write_dsp_boot_addr(rproc);

> +		if (ret)

> +			return ret;

> +	}

>  

>  	client->dev = dev;

>  	client->tx_done = NULL;

> -- 

> 2.17.1

> 

> --

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Bjorn Andersson Nov. 11, 2019, 11:22 p.m. | #5
On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

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

> 

> An implementation for the rproc ops .da_to_va() has been added

> that provides the address translation between device addresses

> to kernel virtual addresses for internal RAMs present on that

> particular remote processor device. The implementation provides

> the translations based on the addresses parsed and stored during

> the probe.

> 

> This ops gets invoked by the exported rproc_da_to_va() function

> and allows the remoteproc core's ELF loader to be able to load

> program data directly into the internal memories.

> 

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

> Signed-off-by: Tero Kristo <t-kristo@ti.com>

> ---

>  drivers/remoteproc/omap_remoteproc.c | 35 ++++++++++++++++++++++++++++

>  1 file changed, 35 insertions(+)

> 

> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

> index bbd6ff360e10..0524f7e0ffa4 100644

> --- a/drivers/remoteproc/omap_remoteproc.c

> +++ b/drivers/remoteproc/omap_remoteproc.c

> @@ -230,10 +230,45 @@ static int omap_rproc_stop(struct rproc *rproc)

>  	return 0;

>  }

>  

> +/*

> + * Internal Memory translation helper


Please format this as kerneldoc.

> + *

> + * Custom function implementing the rproc .da_to_va ops to provide address

> + * translation (device address to kernel virtual address) for internal RAMs

> + * present in a DSP or IPU device). The translated addresses can be used

> + * either by the remoteproc core for loading, or by any rpmsg bus drivers.

> + */

> +static void *omap_rproc_da_to_va(struct rproc *rproc, u64 da, int len)

> +{

> +	struct omap_rproc *oproc = rproc->priv;

> +	void *va = NULL;

> +	int i;

> +	u32 offset;

> +

> +	if (len <= 0)

> +		return NULL;

> +

> +	if (!oproc->num_mems)

> +		return NULL;

> +

> +	for (i = 0; i < oproc->num_mems; i++) {

> +		if (da >= oproc->mem[i].dev_addr && da + len <=

> +		    oproc->mem[i].dev_addr +  oproc->mem[i].size) {

> +			offset = da -  oproc->mem[i].dev_addr;

> +			/* __force to make sparse happy with type conversion */

> +			va = (__force void *)(oproc->mem[i].cpu_addr + offset);


Replace va = and break; with just a return here.

> +			break;

> +		}

> +	}

> +

> +	return va;


return NULL here.

Regards,
Bjorn
Bjorn Andersson Nov. 11, 2019, 11:37 p.m. | #6
On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

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

> 

> DRA7xx/AM57xx SoCs have two IPU and up to two DSP processor subsystems

> for offloading different computation algorithms. The IPU processor

> subsystem contains dual-core ARM Cortex-M4 processors, and is very

> similar to those on OMAP5. The DSP processor subsystem is based on

> the TI's standard TMS320C66x DSP CorePac core.

> 

> Support has been added to the OMAP remoteproc driver through new

> DRA7xx specific compatibles for properly probing and booting the

> all the different processor subsystem instances on DRA7xx/AM57xx

> SoCs - IPU1, IPU2, DSP1 & DSP2. A build dependency with SOC_DRA7XX

> is added to enable the driver to be built in DRA7xx-only configuration.

> 

> The DSP boot address programming needed enhancement for DRA7xx as the

> boot register fields are different on DRA7 compared to OMAP4 and OMAP5

> SoCs. The register on DRA7xx contains additional fields within the

> register and the boot address bit-field is right-shifted by 10 bits.

> The internal memory parsing logic has also been updated to compute

> the device addresses for the L2 RAM for DSP devices using relative

> addressing logic, and to parse two additional RAMs at L1 level - L1P

> and L1D. This allows the remoteproc driver to support loading into

> these regions for a small subset of firmware images requiring as

> such. The most common usage would be to use the L1 programmable

> RAMs as L1 Caches.

> 

> The firmware lookup logic also has to be adjusted for DRA7xx as

> there are (can be) more than one instance of both the IPU and DSP

> remote processors for the first time in OMAP4+ SoCs.

> 

> The names for the firmware images are fixed for each processor and

> are expected to be as follows:

> 	IPU1: dra7-ipu1-fw.xem4

> 	IPU2: dra7-ipu2-fw.xem4

> 	DSP1: dra7-dsp1-fw.xe66

> 	DSP2: dra7-dsp2-fw.xe66

> 

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

> [t-kristo@ti.com: fixed l4_offset calculation logic]

> Signed-off-by: Tero Kristo <t-kristo@ti.com>

> ---

>  drivers/remoteproc/Kconfig           |   2 +-

>  drivers/remoteproc/omap_remoteproc.c | 103 ++++++++++++++++++++++++---

>  2 files changed, 96 insertions(+), 9 deletions(-)

> 

> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig

> index 94afdde4bc9f..d6450d7fcf92 100644

> --- a/drivers/remoteproc/Kconfig

> +++ b/drivers/remoteproc/Kconfig

> @@ -25,7 +25,7 @@ config IMX_REMOTEPROC

>  

>  config OMAP_REMOTEPROC

>  	tristate "OMAP remoteproc support"

> -	depends on ARCH_OMAP4 || SOC_OMAP5

> +	depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX

>  	depends on OMAP_IOMMU

>  	select MAILBOX

>  	select OMAP2PLUS_MBOX

> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

> index 0b80570effee..e46bb4c790d7 100644

> --- a/drivers/remoteproc/omap_remoteproc.c

> +++ b/drivers/remoteproc/omap_remoteproc.c

> @@ -17,6 +17,7 @@

>  #include <linux/module.h>

>  #include <linux/err.h>

>  #include <linux/of_device.h>

> +#include <linux/of_address.h>

>  #include <linux/of_reserved_mem.h>

>  #include <linux/platform_device.h>

>  #include <linux/dma-mapping.h>

> @@ -30,16 +31,20 @@

>  #include "omap_remoteproc.h"

>  #include "remoteproc_internal.h"

>  

> +#define OMAP_RPROC_DSP_LOCAL_MEM_OFFSET		(0x00800000)

>  #define OMAP_RPROC_IPU_L2RAM_DEV_ADDR		(0x20000000)

>  

>  /**

>   * struct omap_rproc_boot_data - boot data structure for the DSP omap rprocs

>   * @syscon: regmap handle for the system control configuration module

>   * @boot_reg: boot register offset within the @syscon regmap

> + * @boot_reg_shift: bit-field shift required for the boot address value in

> + *		    @boot_reg

>   */

>  struct omap_rproc_boot_data {

>  	struct regmap *syscon;

>  	unsigned int boot_reg;

> +	unsigned int boot_reg_shift;

>  };

>  

>  /*

> @@ -151,14 +156,19 @@ static int omap_rproc_write_dsp_boot_addr(struct rproc *rproc)

>  	struct omap_rproc *oproc = rproc->priv;

>  	struct omap_rproc_boot_data *bdata = oproc->boot_data;

>  	u32 offset = bdata->boot_reg;

> +	unsigned int value = rproc->bootaddr;

> +	unsigned int mask = ~(SZ_1K - 1);

>  

> -	if (rproc->bootaddr & (SZ_1K - 1)) {

> +	if (value & (SZ_1K - 1)) {


I would prefer if you keep this as it was.

>  		dev_err(dev, "invalid boot address 0x%x, must be aligned on a 1KB boundary\n",

> -			rproc->bootaddr);

> +			value);


Ditto.

>  		return -EINVAL;

>  	}

>  

> -	regmap_write(bdata->syscon, offset, rproc->bootaddr);

> +	value >>= bdata->boot_reg_shift;

> +	mask >>= bdata->boot_reg_shift;


And then

value = rproc->bootaddr >> bdata->boot_reg_shift;
mask = ~((SZ_1K - 1) >> bdata->boot_reg_shift);

Note though that this would make the upper boot_reg_shift bits 1, while
in your case they are 0s.

> +

> +	regmap_update_bits(bdata->syscon, offset, mask, value);

>  

>  	return 0;

>  }

> @@ -292,6 +302,28 @@ static const struct omap_rproc_dev_data omap5_ipu_dev_data = {

>  	.fw_name	= "omap5-ipu-fw.xem4",

>  };

>  

> +static const struct omap_rproc_dev_data dra7_rproc_dev_data[] = {

> +	{

> +		.device_name	= "40800000.dsp",

> +		.fw_name	= "dra7-dsp1-fw.xe66",

> +	},

> +	{

> +		.device_name	= "41000000.dsp",

> +		.fw_name	= "dra7-dsp2-fw.xe66",

> +	},

> +	{

> +		.device_name	= "55020000.ipu",

> +		.fw_name	= "dra7-ipu2-fw.xem4",

> +	},

> +	{

> +		.device_name	= "58820000.ipu",

> +		.fw_name	= "dra7-ipu1-fw.xem4",

> +	},

> +	{

> +		/* sentinel */

> +	},

> +};

> +

>  static const struct of_device_id omap_rproc_of_match[] = {

>  	{

>  		.compatible     = "ti,omap4-dsp",

> @@ -309,6 +341,14 @@ static const struct of_device_id omap_rproc_of_match[] = {

>  		.compatible     = "ti,omap5-ipu",

>  		.data           = &omap5_ipu_dev_data,

>  	},

> +	{

> +		.compatible     = "ti,dra7-dsp",

> +		.data           = dra7_rproc_dev_data,

> +	},

> +	{

> +		.compatible     = "ti,dra7-ipu",

> +		.data           = dra7_rproc_dev_data,

> +	},

>  	{

>  		/* end */

>  	},

> @@ -317,13 +357,23 @@ MODULE_DEVICE_TABLE(of, omap_rproc_of_match);

>  

>  static const char *omap_rproc_get_firmware(struct platform_device *pdev)

>  {

> +	struct device_node *np = pdev->dev.of_node;

>  	const struct omap_rproc_dev_data *data;

>  

>  	data = of_device_get_match_data(&pdev->dev);

>  	if (!data)

>  		return ERR_PTR(-ENODEV);

>  

> -	return data->fw_name;

> +	if (!of_device_is_compatible(np, "ti,dra7-dsp") &&

> +	    !of_device_is_compatible(np, "ti,dra7-ipu"))

> +		return data->fw_name;

> +

> +	for (; data && data->device_name; data++) {

> +		if (!strcmp(dev_name(&pdev->dev), data->device_name))


I don't fancy the reliance on node names in devicetree, is this well
defined in the binding?

> +			return data->fw_name;

> +	}

> +

> +	return ERR_PTR(-ENOENT);

>  }

>  

>  static int omap_rproc_get_boot_data(struct platform_device *pdev,

> @@ -334,7 +384,8 @@ static int omap_rproc_get_boot_data(struct platform_device *pdev,

>  	int ret;

>  

>  	if (!of_device_is_compatible(np, "ti,omap4-dsp") &&

> -	    !of_device_is_compatible(np, "ti,omap5-dsp"))

> +	    !of_device_is_compatible(np, "ti,omap5-dsp") &&

> +	    !of_device_is_compatible(np, "ti,dra7-dsp"))

>  		return 0;

>  

>  	oproc->boot_data = devm_kzalloc(&pdev->dev, sizeof(*oproc->boot_data),

> @@ -360,18 +411,27 @@ static int omap_rproc_get_boot_data(struct platform_device *pdev,

>  		return -EINVAL;

>  	}

>  

> +	if (of_device_is_compatible(np, "ti,dra7-dsp"))

> +		oproc->boot_data->boot_reg_shift = 10;


Put this in omap_rproc_dev_data.

> +

>  	return 0;

>  }

>  

>  static int omap_rproc_of_get_internal_memories(struct platform_device *pdev,

>  					       struct rproc *rproc)

>  {

> -	static const char * const mem_names[] = {"l2ram"};

> +	static const char * const ipu_mem_names[] = {"l2ram"};

> +	static const char * const dra7_dsp_mem_names[] = {"l2ram", "l1pram",

> +								"l1dram"};

>  	struct device_node *np = pdev->dev.of_node;

>  	struct omap_rproc *oproc = rproc->priv;

>  	struct device *dev = &pdev->dev;

> +	const char * const *mem_names;

>  	struct resource *res;

>  	int num_mems;

> +	const __be32 *addrp;

> +	u32 l4_offset = 0;

> +	u64 size;

>  	int i;

>  

>  	/* OMAP4 and OMAP5 DSPs do not have support for flat SRAM */

> @@ -379,7 +439,15 @@ static int omap_rproc_of_get_internal_memories(struct platform_device *pdev,

>  	    of_device_is_compatible(np, "ti,omap5-dsp"))

>  		return 0;

>  

> -	num_mems = ARRAY_SIZE(mem_names);

> +	/* DRA7 DSPs have two additional SRAMs at L1 level */

> +	if (of_device_is_compatible(np, "ti,dra7-dsp")) {

> +		mem_names = dra7_dsp_mem_names;

> +		num_mems = ARRAY_SIZE(dra7_dsp_mem_names);

> +	} else {

> +		mem_names = ipu_mem_names;

> +		num_mems = ARRAY_SIZE(ipu_mem_names);

> +	}

> +

>  	oproc->mem = devm_kcalloc(dev, num_mems, sizeof(*oproc->mem),

>  				  GFP_KERNEL);

>  	if (!oproc->mem)

> @@ -395,7 +463,26 @@ static int omap_rproc_of_get_internal_memories(struct platform_device *pdev,

>  			return PTR_ERR(oproc->mem[i].cpu_addr);

>  		}

>  		oproc->mem[i].bus_addr = res->start;

> -		oproc->mem[i].dev_addr = OMAP_RPROC_IPU_L2RAM_DEV_ADDR;

> +

> +		/*

> +		 * The DSPs have the internal memories starting at a fixed

> +		 * offset of 0x800000 from address 0, and this corresponds to

> +		 * L2RAM. The L3 address view has the L2RAM bus address as the

> +		 * starting address for the IP, so the L2RAM memory region needs

> +		 * to be processed first, and the device addresses for each

> +		 * memory region can be computed using the relative offset

> +		 * from this base address.

> +		 */

> +		if (of_device_is_compatible(np, "ti,dra7-dsp") &&


Please don't use a ternary operator repeatedly, it makes the code hard
to follow.

Regards,
Bjorn

> +		    !strcmp(mem_names[i], "l2ram")) {

> +			addrp = of_get_address(dev->of_node, i, &size, NULL);

> +			l4_offset = of_translate_address(dev->of_node, addrp);

> +		}

> +		oproc->mem[i].dev_addr =

> +			of_device_is_compatible(np, "ti,dra7-dsp") ?

> +				res->start - l4_offset +

> +				OMAP_RPROC_DSP_LOCAL_MEM_OFFSET :

> +				OMAP_RPROC_IPU_L2RAM_DEV_ADDR;

>  		oproc->mem[i].size = resource_size(res);

>  

>  		dev_dbg(dev, "memory %8s: bus addr %pa size 0x%x va %p da 0x%x\n",

> -- 

> 2.17.1

> 

> --

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Bjorn Andersson Nov. 11, 2019, 11:37 p.m. | #7
On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

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

> 

> The omap_rproc_reserve_cma() function is not defined at the moment.

> This prototype was to be used to define a function to declare a

> remoteproc device-specific CMA pool.

> 

> The remoteproc devices will be defined through DT going forward. A

> device specific CMA pool will be defined under the reserved-memory

> node, and will be associated with the appropriate remoteproc device

> node. This function prototype will no longer be needed and has

> therefore been cleaned up.

> 

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

> Signed-off-by: Tero Kristo <t-kristo@ti.com>


Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


> ---

>  include/linux/platform_data/remoteproc-omap.h | 12 ------------

>  1 file changed, 12 deletions(-)

> 

> diff --git a/include/linux/platform_data/remoteproc-omap.h b/include/linux/platform_data/remoteproc-omap.h

> index 6bea01e199fe..49c78805916f 100644

> --- a/include/linux/platform_data/remoteproc-omap.h

> +++ b/include/linux/platform_data/remoteproc-omap.h

> @@ -21,16 +21,4 @@ struct omap_rproc_pdata {

>  	int (*device_shutdown)(struct platform_device *pdev);

>  };

>  

> -#if defined(CONFIG_OMAP_REMOTEPROC) || defined(CONFIG_OMAP_REMOTEPROC_MODULE)

> -

> -void __init omap_rproc_reserve_cma(void);

> -

> -#else

> -

> -static inline void __init omap_rproc_reserve_cma(void)

> -{

> -}

> -

> -#endif

> -

>  #endif /* _PLAT_REMOTEPROC_H */

> -- 

> 2.17.1

> 

> --

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Bjorn Andersson Nov. 11, 2019, 11:39 p.m. | #8
On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

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

> 

> Add some checks in the mailbox callback function to limit

> any processing in the mailbox callback function to only

> certain currently valid messages, and drop all the remaining

> messages. A debug message is added to print any such invalid

> messages when the appropriate trace control is enabled.

> 

> Signed-off-by: Subramaniam Chanderashekarapuram <subramaniam.ca@ti.com>

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


This should either have a "Co-developed-by" or Suman should be the first
one.

> Signed-off-by: Tero Kristo <t-kristo@ti.com>


Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


Regards,
Bjorn

> ---

>  drivers/remoteproc/omap_remoteproc.c | 6 ++++++

>  drivers/remoteproc/omap_remoteproc.h | 7 +++++++

>  2 files changed, 13 insertions(+)

> 

> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

> index e46bb4c790d7..016d5beda195 100644

> --- a/drivers/remoteproc/omap_remoteproc.c

> +++ b/drivers/remoteproc/omap_remoteproc.c

> @@ -124,6 +124,12 @@ static void omap_rproc_mbox_callback(struct mbox_client *client, void *data)

>  		dev_info(dev, "received echo reply from %s\n", name);

>  		break;

>  	default:

> +		if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG)

> +			return;

> +		if (msg > oproc->rproc->max_notifyid) {

> +			dev_dbg(dev, "dropping unknown message 0x%x", msg);

> +			return;

> +		}

>  		/* msg contains the index of the triggered vring */

>  		if (rproc_vq_interrupt(oproc->rproc, msg) == IRQ_NONE)

>  			dev_dbg(dev, "no message was found in vqid %d\n", msg);

> diff --git a/drivers/remoteproc/omap_remoteproc.h b/drivers/remoteproc/omap_remoteproc.h

> index 1e6fef753c4f..18f522617683 100644

> --- a/drivers/remoteproc/omap_remoteproc.h

> +++ b/drivers/remoteproc/omap_remoteproc.h

> @@ -31,6 +31,12 @@

>   *

>   * @RP_MBOX_ABORT_REQUEST: a "please crash" request, used for testing the

>   * recovery mechanism (to some extent).

> + *

> + * Introduce new message definitions if any here.

> + *

> + * @RP_MBOX_END_MSG: Indicates end of known/defined messages from remote core

> + * This should be the last definition.

> + *

>   */

>  enum omap_rp_mbox_messages {

>  	RP_MBOX_READY		= 0xFFFFFF00,

> @@ -39,6 +45,7 @@ enum omap_rp_mbox_messages {

>  	RP_MBOX_ECHO_REQUEST	= 0xFFFFFF03,

>  	RP_MBOX_ECHO_REPLY	= 0xFFFFFF04,

>  	RP_MBOX_ABORT_REQUEST	= 0xFFFFFF05,

> +	RP_MBOX_END_MSG		= 0xFFFFFF06,

>  };

>  

>  #endif /* _OMAP_RPMSG_H */

> -- 

> 2.17.1

> 

> --

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Bjorn Andersson Nov. 12, 2019, 4:13 a.m. | #9
On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

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

> 

> The remote processors in OMAP4+ SoCs are equipped with internal

> timers, like the internal SysTick timer in a Cortex M3/M4 NVIC or

> the CTM timer within Unicache in IPU & DSP. However, these timers

> are gated when the processor subsystem clock is gated, making

> them rather difficult to use as OS tick sources. They will not

> be able to wakeup the processor from any processor-sleep induced

> clock-gating states.

> 

> This can be avoided by using an external timer as the tick source,

> which can be controlled independently by the OMAP remoteproc

> driver code, but still allowing the processor subsystem clock to

> be auto-gated when the remoteproc cores are idle.

> 

> This patch adds the support for OMAP remote processors to request

> timer(s) to be used by the remoteproc. The timers are enabled and

> disabled in line with the enabling/disabling of the remoteproc.

> The timer data is not mandatory if the advanced device management

> features are not required.

> 

> The core timer functionality is provided by the OMAP DMTimer

> clocksource driver, which does not export any API. The logic is

> implemented through the timer device's platform data ops. The OMAP

> remoteproc driver mainly requires ops to request/free a dmtimer,

> and to start/stop a timer. The split ops helps in controlling the

> timer state without having to request and release a timer everytime

> it needs to use the timer.

> 

> NOTE: If the gptimer is already in use by the time IPU and/or

> DSP are loaded, the processors will fail to boot.

> 

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

> Signed-off-by: Tero Kristo <t-kristo@ti.com>

> ---

>  drivers/remoteproc/omap_remoteproc.c | 258 +++++++++++++++++++++++++++

>  1 file changed, 258 insertions(+)

> 

> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

> index 016d5beda195..8450dd79d391 100644

> --- a/drivers/remoteproc/omap_remoteproc.c

> +++ b/drivers/remoteproc/omap_remoteproc.c

> @@ -27,6 +27,9 @@

>  #include <linux/regmap.h>

>  #include <linux/mfd/syscon.h>

>  #include <linux/reset.h>

> +#include <clocksource/timer-ti-dm.h>

> +

> +#include <linux/platform_data/dmtimer-omap.h>

>  

>  #include "omap_remoteproc.h"

>  #include "remoteproc_internal.h"

> @@ -61,6 +64,16 @@ struct omap_rproc_mem {

>  	size_t size;

>  };

>  

> +/**

> + * struct omap_rproc_timer - data structure for a timer used by a omap rproc

> + * @odt: timer pointer

> + * @timer_ops: OMAP dmtimer ops for @odt timer

> + */

> +struct omap_rproc_timer {

> +	struct omap_dm_timer *odt;

> +	const struct omap_dm_timer_ops *timer_ops;

> +};

> +

>  /**

>   * struct omap_rproc - omap remote processor state

>   * @mbox: mailbox channel handle

> @@ -68,6 +81,8 @@ struct omap_rproc_mem {

>   * @boot_data: boot data structure for setting processor boot address

>   * @mem: internal memory regions data

>   * @num_mems: number of internal memory regions

> + * @num_timers: number of rproc timer(s)

> + * @timers: timer(s) info used by rproc

>   * @rproc: rproc handle

>   * @reset: reset handle

>   */

> @@ -77,6 +92,8 @@ struct omap_rproc {

>  	struct omap_rproc_boot_data *boot_data;

>  	struct omap_rproc_mem *mem;

>  	int num_mems;

> +	int num_timers;

> +	struct omap_rproc_timer *timers;

>  	struct rproc *rproc;

>  	struct reset_control *reset;

>  };

> @@ -91,6 +108,212 @@ struct omap_rproc_dev_data {

>  	const char *fw_name;

>  };

>  

> +/**

> + * omap_rproc_request_timer - request a timer for a remoteproc


Add parenthesis on functions in kerneldoc.

> + * @np: device node pointer to the desired timer

> + * @timer: handle to a struct omap_rproc_timer to return the timer handle

> + *

> + * This helper function is used primarily to request a timer associated with

> + * a remoteproc. The returned handle is stored in the .odt field of the

> + * @timer structure passed in, and is used to invoke other timer specific

> + * ops (like starting a timer either during device initialization or during

> + * a resume operation, or for stopping/freeing a timer).

> + *

> + * Returns 0 on success, otherwise an appropriate failure

> + */

> +static int omap_rproc_request_timer(struct device_node *np,

> +				    struct omap_rproc_timer *timer)

> +{

> +	int ret = 0;

> +

> +	timer->odt = timer->timer_ops->request_by_node(np);

> +	if (!timer->odt) {

> +		pr_err("request for timer node %p failed\n", np);

> +		return -EBUSY;

> +	}

> +

> +	ret = timer->timer_ops->set_source(timer->odt, OMAP_TIMER_SRC_SYS_CLK);

> +	if (ret) {

> +		pr_err("error setting OMAP_TIMER_SRC_SYS_CLK as source for timer node %p\n",

> +		       np);


You could easily pass a struct device * from omap_rproc_enable_timers()
to make this a more useful dev_err()

> +		timer->timer_ops->free(timer->odt);

> +		return ret;

> +	}

> +

> +	/* clean counter, remoteproc code will set the value */

> +	timer->timer_ops->set_load(timer->odt, 0, 0);

> +

> +	return ret;


ret is 0 here, so return 0;

> +}

> +

> +/**

> + * omap_rproc_start_timer - start a timer for a remoteproc

> + * @timer: handle to a OMAP rproc timer

> + *

> + * This helper function is used to start a timer associated with a remoteproc,

> + * obtained using the request_timer ops. The helper function needs to be

> + * invoked by the driver to start the timer (during device initialization)

> + * or to just resume the timer.

> + *

> + * Returns 0 on success, otherwise a failure as returned by DMTimer ops

> + */

> +static inline int omap_rproc_start_timer(struct omap_rproc_timer *timer)

> +{

> +	return timer->timer_ops->start(timer->odt);

> +}

> +

> +/**

> + * omap_rproc_stop_timer - stop a timer for a remoteproc

> + * @timer: handle to a OMAP rproc timer

> + *

> + * This helper function is used to disable a timer associated with a

> + * remoteproc, and needs to be called either during a device shutdown

> + * or suspend operation. The separate helper function allows the driver

> + * to just stop a timer without having to release the timer during a

> + * suspend operation.

> + *

> + * Returns 0 on success, otherwise a failure as returned by DMTimer ops

> + */

> +static inline int omap_rproc_stop_timer(struct omap_rproc_timer *timer)

> +{

> +	return timer->timer_ops->stop(timer->odt);

> +}

> +

> +/**

> + * omap_rproc_release_timer - release a timer for a remoteproc

> + * @timer: handle to a OMAP rproc timer

> + *

> + * This helper function is used primarily to release a timer associated

> + * with a remoteproc. The dmtimer will be available for other clients to

> + * use once released.

> + *

> + * Returns 0 on success, otherwise a failure as returned by DMTimer ops

> + */

> +static inline int omap_rproc_release_timer(struct omap_rproc_timer *timer)

> +{

> +	return timer->timer_ops->free(timer->odt);

> +}

> +

> +/**

> + * omap_rproc_enable_timers - enable the timers for a remoteproc

> + * @rproc: handle of a remote processor

> + * @configure: boolean flag used to acquire and configure the timer handle

> + *

> + * This function is used primarily to enable the timers associated with

> + * a remoteproc. The configure flag is provided to allow the driver to

> + * to either acquire and start a timer (during device initialization) or

> + * to just start a timer (during a resume operation).

> + */

> +static int omap_rproc_enable_timers(struct rproc *rproc, bool configure)

> +{

> +	int i;

> +	int ret = 0;

> +	struct platform_device *tpdev;

> +	struct dmtimer_platform_data *tpdata;

> +	const struct omap_dm_timer_ops *timer_ops;

> +	struct omap_rproc *oproc = rproc->priv;

> +	struct omap_rproc_timer *timers = oproc->timers;

> +	struct device *dev = rproc->dev.parent;

> +	struct device_node *np = NULL;

> +

> +	if (oproc->num_timers <= 0)

> +		return 0;

> +

> +	if (!configure)

> +		goto start_timers;

> +

> +	for (i = 0; i < oproc->num_timers; i++) {

> +		np = of_parse_phandle(dev->of_node, "timers", i);

> +		if (!np) {

> +			ret = -ENXIO;

> +			dev_err(dev, "device node lookup for timer at index %d failed: %d\n",

> +				i, ret);

> +			goto free_timers;

> +		}

> +

> +		tpdev = of_find_device_by_node(np);

> +		if (!tpdev) {

> +			ret = -ENODEV;

> +			dev_err(dev, "could not get timer platform device\n");

> +			goto put_node;

> +		}

> +

> +		tpdata = dev_get_platdata(&tpdev->dev);

> +		put_device(&tpdev->dev);

> +		if (!tpdata) {

> +			ret = -EINVAL;

> +			dev_err(dev, "dmtimer pdata structure NULL\n");

> +			goto put_node;

> +		}

> +

> +		timer_ops = tpdata->timer_ops;

> +		if (!timer_ops || !timer_ops->request_by_node ||

> +		    !timer_ops->set_source || !timer_ops->set_load ||

> +		    !timer_ops->free || !timer_ops->start ||

> +		    !timer_ops->stop) {

> +			ret = -EINVAL;

> +			dev_err(dev, "device does not have required timer ops\n");

> +			goto put_node;

> +		}

> +

> +		timers[i].timer_ops = timer_ops;

> +		ret = omap_rproc_request_timer(np, &timers[i]);

> +		if (ret) {

> +			dev_err(dev, "request for timer %p failed: %d\n", np,

> +				ret);

> +			goto put_node;

> +		}

> +		of_node_put(np);

> +	}

> +

> +start_timers:

> +	for (i = 0; i < oproc->num_timers; i++)

> +		omap_rproc_start_timer(&timers[i]);

> +	return 0;

> +

> +put_node:

> +	of_node_put(np);

> +free_timers:

> +	while (i--) {

> +		omap_rproc_release_timer(&timers[i]);

> +		timers[i].odt = NULL;

> +		timers[i].timer_ops = NULL;

> +	}

> +

> +	return ret;

> +}

> +

> +/**

> + * omap_rproc_disable_timers - disable the timers for a remoteproc

> + * @rproc: handle of a remote processor

> + * @configure: boolean flag used to release the timer handle

> + *

> + * This function is used primarily to disable the timers associated with

> + * a remoteproc. The configure flag is provided to allow the driver to

> + * to either stop and release a timer (during device shutdown) or to just

> + * stop a timer (during a suspend operation).

> + */

> +static int omap_rproc_disable_timers(struct rproc *rproc, bool configure)

> +{

> +	int i;

> +	struct omap_rproc *oproc = rproc->priv;

> +	struct omap_rproc_timer *timers = oproc->timers;

> +

> +	if (oproc->num_timers <= 0)

> +		return 0;

> +

> +	for (i = 0; i < oproc->num_timers; i++) {

> +		omap_rproc_stop_timer(&timers[i]);

> +		if (configure) {

> +			omap_rproc_release_timer(&timers[i]);

> +			timers[i].odt = NULL;

> +			timers[i].timer_ops = NULL;

> +		}

> +	}

> +

> +	return 0;

> +}

> +

>  /**

>   * omap_rproc_mbox_callback() - inbound mailbox message handler

>   * @client: mailbox client pointer used for requesting the mailbox channel

> @@ -226,6 +449,12 @@ static int omap_rproc_start(struct rproc *rproc)

>  		goto put_mbox;

>  	}

>  

> +	ret = omap_rproc_enable_timers(rproc, true);

> +	if (ret) {

> +		dev_err(dev, "omap_rproc_enable_timers failed: %d\n", ret);

> +		goto put_mbox;

> +	}

> +

>  	reset_control_deassert(oproc->reset);

>  

>  	return 0;

> @@ -239,9 +468,14 @@ static int omap_rproc_start(struct rproc *rproc)

>  static int omap_rproc_stop(struct rproc *rproc)

>  {

>  	struct omap_rproc *oproc = rproc->priv;

> +	int ret;

>  

>  	reset_control_assert(oproc->reset);

>  

> +	ret = omap_rproc_disable_timers(rproc, true);

> +	if (ret)

> +		return ret;

> +

>  	mbox_free_channel(oproc->mbox);

>  

>  	return 0;

> @@ -548,6 +782,30 @@ static int omap_rproc_probe(struct platform_device *pdev)

>  	if (ret)

>  		goto free_rproc;

>  

> +	/*

> +	 * Timer nodes are directly used in client nodes as phandles, so

> +	 * retrieve the count using appropriate size

> +	 */

> +	oproc->num_timers = of_property_count_elems_of_size(np, "timers",


Didn't this get a ti, prefix?

And I think you should use of_count_phandle_with_args() instead.

Regards,
Bjorn

> +							    sizeof(phandle));

> +	if (oproc->num_timers <= 0) {

> +		dev_dbg(&pdev->dev, "device does not have timers, status = %d\n",

> +			oproc->num_timers);

> +		oproc->num_timers = 0;

> +	}

> +

> +	if (oproc->num_timers) {

> +		oproc->timers = devm_kzalloc(&pdev->dev, sizeof(*oproc->timers)

> +					     * oproc->num_timers, GFP_KERNEL);

> +		if (!oproc->timers) {

> +			ret = -ENOMEM;

> +			goto free_rproc;

> +		}

> +

> +		dev_dbg(&pdev->dev, "device has %d tick timers\n",

> +			oproc->num_timers);

> +	}

> +

>  	ret = of_reserved_mem_device_init(&pdev->dev);

>  	if (ret) {

>  		dev_err(&pdev->dev, "device does not have specific CMA pool\n");

> -- 

> 2.17.1

> 

> --

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Bjorn Andersson Nov. 12, 2019, 6:15 a.m. | #10
On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:
> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

[..]
> +static int _omap_rproc_suspend(struct rproc *rproc)


I think it would make sense to inline this and _omap_rproc_resume() in
their single call sites.

[..]
> +static int _omap_rproc_resume(struct rproc *rproc)

> +{

[..]
> @@ -806,6 +972,14 @@ static int omap_rproc_probe(struct platform_device *pdev)

>  			oproc->num_timers);

>  	}

>  

> +	init_completion(&oproc->pm_comp);

> +

> +	oproc->fck = of_clk_get(np, 0);


devm_clk_get() ? 

Otherwise I think you're lacking a clk_put() in omap_rproc_remove()

Regards,
Bjorn
Tero Kristo Nov. 12, 2019, 8:21 a.m. | #11
On 12/11/2019 01:21, Bjorn Andersson wrote:
> On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

> 

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

>>

>> The OMAP remoteproc driver has been enhanced to parse and store

>> the kernel mappings for different internal RAM memories that may

>> be present within each remote processor IP subsystem. Different

>> devices have varying memories present on current SoCs. The current

>> support handles the L2RAM for all IPU devices on OMAP4+ SoCs. The

>> DSPs on OMAP4/OMAP5 only have Unicaches and do not have any L1 or

>> L2 RAM memories.

>>

>> IPUs are expected to have the L2RAM at a fixed device address of

>> 0x20000000, based on the current limitations on Attribute MMU

>> configurations.

>>

>> NOTE:

>> The current logic doesn't handle the parsing of memories for DRA7

>> remoteproc devices, and will be added alongside the DRA7 support.

>>

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

>> Signed-off-by: Tero Kristo <t-kristo@ti.com>

>> ---

>>   drivers/remoteproc/omap_remoteproc.c | 69 ++++++++++++++++++++++++++++

>>   1 file changed, 69 insertions(+)

>>

>> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

>> index a10377547533..bbd6ff360e10 100644

>> --- a/drivers/remoteproc/omap_remoteproc.c

>> +++ b/drivers/remoteproc/omap_remoteproc.c

>> @@ -29,6 +29,8 @@

>>   #include "omap_remoteproc.h"

>>   #include "remoteproc_internal.h"

>>   

>> +#define OMAP_RPROC_IPU_L2RAM_DEV_ADDR		(0x20000000)

>> +

>>   /**

>>    * struct omap_rproc_boot_data - boot data structure for the DSP omap rprocs

>>    * @syscon: regmap handle for the system control configuration module

>> @@ -39,11 +41,27 @@ struct omap_rproc_boot_data {

>>   	unsigned int boot_reg;

>>   };

>>   

>> +/*

>> + * struct omap_rproc_mem - internal memory structure

>> + * @cpu_addr: MPU virtual address of the memory region

>> + * @bus_addr: bus address used to access the memory region

>> + * @dev_addr: device address of the memory region from DSP view

>> + * @size: size of the memory region

>> + */

>> +struct omap_rproc_mem {

>> +	void __iomem *cpu_addr;

>> +	phys_addr_t bus_addr;

>> +	u32 dev_addr;

>> +	size_t size;

>> +};

>> +

>>   /**

>>    * struct omap_rproc - omap remote processor state

>>    * @mbox: mailbox channel handle

>>    * @client: mailbox client to request the mailbox channel

>>    * @boot_data: boot data structure for setting processor boot address

>> + * @mem: internal memory regions data

>> + * @num_mems: number of internal memory regions

>>    * @rproc: rproc handle

>>    * @reset: reset handle

>>    */

>> @@ -51,6 +69,8 @@ struct omap_rproc {

>>   	struct mbox_chan *mbox;

>>   	struct mbox_client client;

>>   	struct omap_rproc_boot_data *boot_data;

>> +	struct omap_rproc_mem *mem;

>> +	int num_mems;

>>   	struct rproc *rproc;

>>   	struct reset_control *reset;

>>   };

>> @@ -307,6 +327,51 @@ static int omap_rproc_get_boot_data(struct platform_device *pdev,

>>   	return 0;

>>   }

>>   

>> +static int omap_rproc_of_get_internal_memories(struct platform_device *pdev,

>> +					       struct rproc *rproc)

>> +{

>> +	static const char * const mem_names[] = {"l2ram"};

>> +	struct device_node *np = pdev->dev.of_node;

>> +	struct omap_rproc *oproc = rproc->priv;

>> +	struct device *dev = &pdev->dev;

>> +	struct resource *res;

>> +	int num_mems;

>> +	int i;

>> +

>> +	/* OMAP4 and OMAP5 DSPs do not have support for flat SRAM */

>> +	if (of_device_is_compatible(np, "ti,omap4-dsp") ||

>> +	    of_device_is_compatible(np, "ti,omap5-dsp"))

>> +		return 0;

>> +

>> +	num_mems = ARRAY_SIZE(mem_names);

>> +	oproc->mem = devm_kcalloc(dev, num_mems, sizeof(*oproc->mem),

>> +				  GFP_KERNEL);

>> +	if (!oproc->mem)

>> +		return -ENOMEM;

>> +

>> +	for (i = 0; i < num_mems; i++) {

>> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,

>> +						   mem_names[i]);

>> +		oproc->mem[i].cpu_addr = devm_ioremap_resource(dev, res);

>> +		if (IS_ERR(oproc->mem[i].cpu_addr)) {

>> +			dev_err(dev, "failed to parse and map %s memory\n",

>> +				mem_names[i]);

>> +			return PTR_ERR(oproc->mem[i].cpu_addr);

>> +		}

>> +		oproc->mem[i].bus_addr = res->start;

>> +		oproc->mem[i].dev_addr = OMAP_RPROC_IPU_L2RAM_DEV_ADDR;

> 

> Presumably this means that mem_names[] will only ever be {"l2ram"} ?

> 

> This would imply that you can either remove the loop or you should

> generalize this for dev_addr as well.


Well, actually support for dra7 remoteprocs is added in patch #8, which 
adds also l1pram/l1dram to the mix.

-Tero

> 

> 

> Apart from that, this looks good.

> 

> Regards,

> Bjorn

> 

>> +		oproc->mem[i].size = resource_size(res);

>> +

>> +		dev_dbg(dev, "memory %8s: bus addr %pa size 0x%x va %p da 0x%x\n",

>> +			mem_names[i], &oproc->mem[i].bus_addr,

>> +			oproc->mem[i].size, oproc->mem[i].cpu_addr,

>> +			oproc->mem[i].dev_addr);

>> +	}

>> +	oproc->num_mems = num_mems;

>> +

>> +	return 0;

>> +}

>> +

>>   static int omap_rproc_probe(struct platform_device *pdev)

>>   {

>>   	struct device_node *np = pdev->dev.of_node;

>> @@ -346,6 +411,10 @@ static int omap_rproc_probe(struct platform_device *pdev)

>>   	/* All existing OMAP IPU and DSP processors have an MMU */

>>   	rproc->has_iommu = true;

>>   

>> +	ret = omap_rproc_of_get_internal_memories(pdev, rproc);

>> +	if (ret)

>> +		goto free_rproc;

>> +

>>   	ret = omap_rproc_get_boot_data(pdev, rproc);

>>   	if (ret)

>>   		goto free_rproc;

>> -- 

>> 2.17.1

>>

>> --


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Tero Kristo Nov. 12, 2019, 8:22 a.m. | #12
On 12/11/2019 01:22, Bjorn Andersson wrote:
> On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

> 

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

>>

>> An implementation for the rproc ops .da_to_va() has been added

>> that provides the address translation between device addresses

>> to kernel virtual addresses for internal RAMs present on that

>> particular remote processor device. The implementation provides

>> the translations based on the addresses parsed and stored during

>> the probe.

>>

>> This ops gets invoked by the exported rproc_da_to_va() function

>> and allows the remoteproc core's ELF loader to be able to load

>> program data directly into the internal memories.

>>

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

>> Signed-off-by: Tero Kristo <t-kristo@ti.com>

>> ---

>>   drivers/remoteproc/omap_remoteproc.c | 35 ++++++++++++++++++++++++++++

>>   1 file changed, 35 insertions(+)

>>

>> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

>> index bbd6ff360e10..0524f7e0ffa4 100644

>> --- a/drivers/remoteproc/omap_remoteproc.c

>> +++ b/drivers/remoteproc/omap_remoteproc.c

>> @@ -230,10 +230,45 @@ static int omap_rproc_stop(struct rproc *rproc)

>>   	return 0;

>>   }

>>   

>> +/*

>> + * Internal Memory translation helper

> 

> Please format this as kerneldoc.

> 

>> + *

>> + * Custom function implementing the rproc .da_to_va ops to provide address

>> + * translation (device address to kernel virtual address) for internal RAMs

>> + * present in a DSP or IPU device). The translated addresses can be used

>> + * either by the remoteproc core for loading, or by any rpmsg bus drivers.

>> + */

>> +static void *omap_rproc_da_to_va(struct rproc *rproc, u64 da, int len)

>> +{

>> +	struct omap_rproc *oproc = rproc->priv;

>> +	void *va = NULL;

>> +	int i;

>> +	u32 offset;

>> +

>> +	if (len <= 0)

>> +		return NULL;

>> +

>> +	if (!oproc->num_mems)

>> +		return NULL;

>> +

>> +	for (i = 0; i < oproc->num_mems; i++) {

>> +		if (da >= oproc->mem[i].dev_addr && da + len <=

>> +		    oproc->mem[i].dev_addr +  oproc->mem[i].size) {

>> +			offset = da -  oproc->mem[i].dev_addr;

>> +			/* __force to make sparse happy with type conversion */

>> +			va = (__force void *)(oproc->mem[i].cpu_addr + offset);

> 

> Replace va = and break; with just a return here.


Right, va seems to be completely redundant local variable.

> 

>> +			break;

>> +		}

>> +	}

>> +

>> +	return va;

> 

> return NULL here.


Yep will do.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Tero Kristo Nov. 12, 2019, 8:42 a.m. | #13
On 12/11/2019 06:13, Bjorn Andersson wrote:
> On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

> 

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

>>

>> The remote processors in OMAP4+ SoCs are equipped with internal

>> timers, like the internal SysTick timer in a Cortex M3/M4 NVIC or

>> the CTM timer within Unicache in IPU & DSP. However, these timers

>> are gated when the processor subsystem clock is gated, making

>> them rather difficult to use as OS tick sources. They will not

>> be able to wakeup the processor from any processor-sleep induced

>> clock-gating states.

>>

>> This can be avoided by using an external timer as the tick source,

>> which can be controlled independently by the OMAP remoteproc

>> driver code, but still allowing the processor subsystem clock to

>> be auto-gated when the remoteproc cores are idle.

>>

>> This patch adds the support for OMAP remote processors to request

>> timer(s) to be used by the remoteproc. The timers are enabled and

>> disabled in line with the enabling/disabling of the remoteproc.

>> The timer data is not mandatory if the advanced device management

>> features are not required.

>>

>> The core timer functionality is provided by the OMAP DMTimer

>> clocksource driver, which does not export any API. The logic is

>> implemented through the timer device's platform data ops. The OMAP

>> remoteproc driver mainly requires ops to request/free a dmtimer,

>> and to start/stop a timer. The split ops helps in controlling the

>> timer state without having to request and release a timer everytime

>> it needs to use the timer.

>>

>> NOTE: If the gptimer is already in use by the time IPU and/or

>> DSP are loaded, the processors will fail to boot.

>>

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

>> Signed-off-by: Tero Kristo <t-kristo@ti.com>

>> ---

>>   drivers/remoteproc/omap_remoteproc.c | 258 +++++++++++++++++++++++++++

>>   1 file changed, 258 insertions(+)

>>

>> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

>> index 016d5beda195..8450dd79d391 100644

>> --- a/drivers/remoteproc/omap_remoteproc.c

>> +++ b/drivers/remoteproc/omap_remoteproc.c

>> @@ -27,6 +27,9 @@

>>   #include <linux/regmap.h>

>>   #include <linux/mfd/syscon.h>

>>   #include <linux/reset.h>

>> +#include <clocksource/timer-ti-dm.h>

>> +

>> +#include <linux/platform_data/dmtimer-omap.h>

>>   

>>   #include "omap_remoteproc.h"

>>   #include "remoteproc_internal.h"

>> @@ -61,6 +64,16 @@ struct omap_rproc_mem {

>>   	size_t size;

>>   };

>>   

>> +/**

>> + * struct omap_rproc_timer - data structure for a timer used by a omap rproc

>> + * @odt: timer pointer

>> + * @timer_ops: OMAP dmtimer ops for @odt timer

>> + */

>> +struct omap_rproc_timer {

>> +	struct omap_dm_timer *odt;

>> +	const struct omap_dm_timer_ops *timer_ops;

>> +};

>> +

>>   /**

>>    * struct omap_rproc - omap remote processor state

>>    * @mbox: mailbox channel handle

>> @@ -68,6 +81,8 @@ struct omap_rproc_mem {

>>    * @boot_data: boot data structure for setting processor boot address

>>    * @mem: internal memory regions data

>>    * @num_mems: number of internal memory regions

>> + * @num_timers: number of rproc timer(s)

>> + * @timers: timer(s) info used by rproc

>>    * @rproc: rproc handle

>>    * @reset: reset handle

>>    */

>> @@ -77,6 +92,8 @@ struct omap_rproc {

>>   	struct omap_rproc_boot_data *boot_data;

>>   	struct omap_rproc_mem *mem;

>>   	int num_mems;

>> +	int num_timers;

>> +	struct omap_rproc_timer *timers;

>>   	struct rproc *rproc;

>>   	struct reset_control *reset;

>>   };

>> @@ -91,6 +108,212 @@ struct omap_rproc_dev_data {

>>   	const char *fw_name;

>>   };

>>   

>> +/**

>> + * omap_rproc_request_timer - request a timer for a remoteproc

> 

> Add parenthesis on functions in kerneldoc.


Ok.

> 

>> + * @np: device node pointer to the desired timer

>> + * @timer: handle to a struct omap_rproc_timer to return the timer handle

>> + *

>> + * This helper function is used primarily to request a timer associated with

>> + * a remoteproc. The returned handle is stored in the .odt field of the

>> + * @timer structure passed in, and is used to invoke other timer specific

>> + * ops (like starting a timer either during device initialization or during

>> + * a resume operation, or for stopping/freeing a timer).

>> + *

>> + * Returns 0 on success, otherwise an appropriate failure

>> + */

>> +static int omap_rproc_request_timer(struct device_node *np,

>> +				    struct omap_rproc_timer *timer)

>> +{

>> +	int ret = 0;

>> +

>> +	timer->odt = timer->timer_ops->request_by_node(np);

>> +	if (!timer->odt) {

>> +		pr_err("request for timer node %p failed\n", np);

>> +		return -EBUSY;

>> +	}

>> +

>> +	ret = timer->timer_ops->set_source(timer->odt, OMAP_TIMER_SRC_SYS_CLK);

>> +	if (ret) {

>> +		pr_err("error setting OMAP_TIMER_SRC_SYS_CLK as source for timer node %p\n",

>> +		       np);

> 

> You could easily pass a struct device * from omap_rproc_enable_timers()

> to make this a more useful dev_err()


True, let me fix that.

> 

>> +		timer->timer_ops->free(timer->odt);

>> +		return ret;

>> +	}

>> +

>> +	/* clean counter, remoteproc code will set the value */

>> +	timer->timer_ops->set_load(timer->odt, 0, 0);

>> +

>> +	return ret;

> 

> ret is 0 here, so return 0;


Right, will fix.

> 

>> +}

>> +

>> +/**

>> + * omap_rproc_start_timer - start a timer for a remoteproc

>> + * @timer: handle to a OMAP rproc timer

>> + *

>> + * This helper function is used to start a timer associated with a remoteproc,

>> + * obtained using the request_timer ops. The helper function needs to be

>> + * invoked by the driver to start the timer (during device initialization)

>> + * or to just resume the timer.

>> + *

>> + * Returns 0 on success, otherwise a failure as returned by DMTimer ops

>> + */

>> +static inline int omap_rproc_start_timer(struct omap_rproc_timer *timer)

>> +{

>> +	return timer->timer_ops->start(timer->odt);

>> +}

>> +

>> +/**

>> + * omap_rproc_stop_timer - stop a timer for a remoteproc

>> + * @timer: handle to a OMAP rproc timer

>> + *

>> + * This helper function is used to disable a timer associated with a

>> + * remoteproc, and needs to be called either during a device shutdown

>> + * or suspend operation. The separate helper function allows the driver

>> + * to just stop a timer without having to release the timer during a

>> + * suspend operation.

>> + *

>> + * Returns 0 on success, otherwise a failure as returned by DMTimer ops

>> + */

>> +static inline int omap_rproc_stop_timer(struct omap_rproc_timer *timer)

>> +{

>> +	return timer->timer_ops->stop(timer->odt);

>> +}

>> +

>> +/**

>> + * omap_rproc_release_timer - release a timer for a remoteproc

>> + * @timer: handle to a OMAP rproc timer

>> + *

>> + * This helper function is used primarily to release a timer associated

>> + * with a remoteproc. The dmtimer will be available for other clients to

>> + * use once released.

>> + *

>> + * Returns 0 on success, otherwise a failure as returned by DMTimer ops

>> + */

>> +static inline int omap_rproc_release_timer(struct omap_rproc_timer *timer)

>> +{

>> +	return timer->timer_ops->free(timer->odt);

>> +}

>> +

>> +/**

>> + * omap_rproc_enable_timers - enable the timers for a remoteproc

>> + * @rproc: handle of a remote processor

>> + * @configure: boolean flag used to acquire and configure the timer handle

>> + *

>> + * This function is used primarily to enable the timers associated with

>> + * a remoteproc. The configure flag is provided to allow the driver to

>> + * to either acquire and start a timer (during device initialization) or

>> + * to just start a timer (during a resume operation).

>> + */

>> +static int omap_rproc_enable_timers(struct rproc *rproc, bool configure)

>> +{

>> +	int i;

>> +	int ret = 0;

>> +	struct platform_device *tpdev;

>> +	struct dmtimer_platform_data *tpdata;

>> +	const struct omap_dm_timer_ops *timer_ops;

>> +	struct omap_rproc *oproc = rproc->priv;

>> +	struct omap_rproc_timer *timers = oproc->timers;

>> +	struct device *dev = rproc->dev.parent;

>> +	struct device_node *np = NULL;

>> +

>> +	if (oproc->num_timers <= 0)

>> +		return 0;

>> +

>> +	if (!configure)

>> +		goto start_timers;

>> +

>> +	for (i = 0; i < oproc->num_timers; i++) {

>> +		np = of_parse_phandle(dev->of_node, "timers", i);

>> +		if (!np) {

>> +			ret = -ENXIO;

>> +			dev_err(dev, "device node lookup for timer at index %d failed: %d\n",

>> +				i, ret);

>> +			goto free_timers;

>> +		}

>> +

>> +		tpdev = of_find_device_by_node(np);

>> +		if (!tpdev) {

>> +			ret = -ENODEV;

>> +			dev_err(dev, "could not get timer platform device\n");

>> +			goto put_node;

>> +		}

>> +

>> +		tpdata = dev_get_platdata(&tpdev->dev);

>> +		put_device(&tpdev->dev);

>> +		if (!tpdata) {

>> +			ret = -EINVAL;

>> +			dev_err(dev, "dmtimer pdata structure NULL\n");

>> +			goto put_node;

>> +		}

>> +

>> +		timer_ops = tpdata->timer_ops;

>> +		if (!timer_ops || !timer_ops->request_by_node ||

>> +		    !timer_ops->set_source || !timer_ops->set_load ||

>> +		    !timer_ops->free || !timer_ops->start ||

>> +		    !timer_ops->stop) {

>> +			ret = -EINVAL;

>> +			dev_err(dev, "device does not have required timer ops\n");

>> +			goto put_node;

>> +		}

>> +

>> +		timers[i].timer_ops = timer_ops;

>> +		ret = omap_rproc_request_timer(np, &timers[i]);

>> +		if (ret) {

>> +			dev_err(dev, "request for timer %p failed: %d\n", np,

>> +				ret);

>> +			goto put_node;

>> +		}

>> +		of_node_put(np);

>> +	}

>> +

>> +start_timers:

>> +	for (i = 0; i < oproc->num_timers; i++)

>> +		omap_rproc_start_timer(&timers[i]);

>> +	return 0;

>> +

>> +put_node:

>> +	of_node_put(np);

>> +free_timers:

>> +	while (i--) {

>> +		omap_rproc_release_timer(&timers[i]);

>> +		timers[i].odt = NULL;

>> +		timers[i].timer_ops = NULL;

>> +	}

>> +

>> +	return ret;

>> +}

>> +

>> +/**

>> + * omap_rproc_disable_timers - disable the timers for a remoteproc

>> + * @rproc: handle of a remote processor

>> + * @configure: boolean flag used to release the timer handle

>> + *

>> + * This function is used primarily to disable the timers associated with

>> + * a remoteproc. The configure flag is provided to allow the driver to

>> + * to either stop and release a timer (during device shutdown) or to just

>> + * stop a timer (during a suspend operation).

>> + */

>> +static int omap_rproc_disable_timers(struct rproc *rproc, bool configure)

>> +{

>> +	int i;

>> +	struct omap_rproc *oproc = rproc->priv;

>> +	struct omap_rproc_timer *timers = oproc->timers;

>> +

>> +	if (oproc->num_timers <= 0)

>> +		return 0;

>> +

>> +	for (i = 0; i < oproc->num_timers; i++) {

>> +		omap_rproc_stop_timer(&timers[i]);

>> +		if (configure) {

>> +			omap_rproc_release_timer(&timers[i]);

>> +			timers[i].odt = NULL;

>> +			timers[i].timer_ops = NULL;

>> +		}

>> +	}

>> +

>> +	return 0;

>> +}

>> +

>>   /**

>>    * omap_rproc_mbox_callback() - inbound mailbox message handler

>>    * @client: mailbox client pointer used for requesting the mailbox channel

>> @@ -226,6 +449,12 @@ static int omap_rproc_start(struct rproc *rproc)

>>   		goto put_mbox;

>>   	}

>>   

>> +	ret = omap_rproc_enable_timers(rproc, true);

>> +	if (ret) {

>> +		dev_err(dev, "omap_rproc_enable_timers failed: %d\n", ret);

>> +		goto put_mbox;

>> +	}

>> +

>>   	reset_control_deassert(oproc->reset);

>>   

>>   	return 0;

>> @@ -239,9 +468,14 @@ static int omap_rproc_start(struct rproc *rproc)

>>   static int omap_rproc_stop(struct rproc *rproc)

>>   {

>>   	struct omap_rproc *oproc = rproc->priv;

>> +	int ret;

>>   

>>   	reset_control_assert(oproc->reset);

>>   

>> +	ret = omap_rproc_disable_timers(rproc, true);

>> +	if (ret)

>> +		return ret;

>> +

>>   	mbox_free_channel(oproc->mbox);

>>   

>>   	return 0;

>> @@ -548,6 +782,30 @@ static int omap_rproc_probe(struct platform_device *pdev)

>>   	if (ret)

>>   		goto free_rproc;

>>   

>> +	/*

>> +	 * Timer nodes are directly used in client nodes as phandles, so

>> +	 * retrieve the count using appropriate size

>> +	 */

>> +	oproc->num_timers = of_property_count_elems_of_size(np, "timers",

> 

> Didn't this get a ti, prefix?


Yes, will change that.

> And I think you should use of_count_phandle_with_args() instead.


True, will fix that also.

-Tero

> 

> Regards,

> Bjorn

> 

>> +							    sizeof(phandle));

>> +	if (oproc->num_timers <= 0) {

>> +		dev_dbg(&pdev->dev, "device does not have timers, status = %d\n",

>> +			oproc->num_timers);

>> +		oproc->num_timers = 0;

>> +	}

>> +

>> +	if (oproc->num_timers) {

>> +		oproc->timers = devm_kzalloc(&pdev->dev, sizeof(*oproc->timers)

>> +					     * oproc->num_timers, GFP_KERNEL);

>> +		if (!oproc->timers) {

>> +			ret = -ENOMEM;

>> +			goto free_rproc;

>> +		}

>> +

>> +		dev_dbg(&pdev->dev, "device has %d tick timers\n",

>> +			oproc->num_timers);

>> +	}

>> +

>>   	ret = of_reserved_mem_device_init(&pdev->dev);

>>   	if (ret) {

>>   		dev_err(&pdev->dev, "device does not have specific CMA pool\n");

>> -- 

>> 2.17.1

>>

>> --


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Tero Kristo Nov. 12, 2019, 8:45 a.m. | #14
On 12/11/2019 08:15, Bjorn Andersson wrote:
> On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

>> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

> [..]

>> +static int _omap_rproc_suspend(struct rproc *rproc)

> 

> I think it would make sense to inline this and _omap_rproc_resume() in

> their single call sites.


Well, these get re-used in following patch for runtime PM also, so it is 
probably better leave this for compiler to decide?

> 

> [..]

>> +static int _omap_rproc_resume(struct rproc *rproc)

>> +{

> [..]

>> @@ -806,6 +972,14 @@ static int omap_rproc_probe(struct platform_device *pdev)

>>   			oproc->num_timers);

>>   	}

>>   

>> +	init_completion(&oproc->pm_comp);

>> +

>> +	oproc->fck = of_clk_get(np, 0);

> 

> devm_clk_get() ?

> 

> Otherwise I think you're lacking a clk_put() in omap_rproc_remove()


Yeah, let me replace with devm_clk_get.

-Tero

> 

> Regards,

> Bjorn

> 


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Bjorn Andersson Nov. 12, 2019, 6:06 p.m. | #15
On Tue 12 Nov 00:45 PST 2019, Tero Kristo wrote:

> On 12/11/2019 08:15, Bjorn Andersson wrote:

> > On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

> > > diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c

> > [..]

> > > +static int _omap_rproc_suspend(struct rproc *rproc)

> > 

> > I think it would make sense to inline this and _omap_rproc_resume() in

> > their single call sites.

> 

> Well, these get re-used in following patch for runtime PM also, so it is

> probably better leave this for compiler to decide?

> 


I didn't see that until later, this is fine.

Regards,
Bjorn

> > 

> > [..]

> > > +static int _omap_rproc_resume(struct rproc *rproc)

> > > +{

> > [..]

> > > @@ -806,6 +972,14 @@ static int omap_rproc_probe(struct platform_device *pdev)

> > >   			oproc->num_timers);

> > >   	}

> > > +	init_completion(&oproc->pm_comp);

> > > +

> > > +	oproc->fck = of_clk_get(np, 0);

> > 

> > devm_clk_get() ?

> > 

> > Otherwise I think you're lacking a clk_put() in omap_rproc_remove()

> 

> Yeah, let me replace with devm_clk_get.

> 

> -Tero

> 

> > 

> > Regards,

> > Bjorn

> > 

> 

> --

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Tero Kristo Nov. 13, 2019, 8:08 a.m. | #16
On 12/11/2019 20:13, Bjorn Andersson wrote:
> On Tue 12 Nov 00:37 PST 2019, Tero Kristo wrote:

> 

>> On 12/11/2019 01:37, Bjorn Andersson wrote:

>>> On Mon 28 Oct 05:42 PDT 2019, Tero Kristo wrote:

> [..]

>>>> +	for (; data && data->device_name; data++) {

>>>> +		if (!strcmp(dev_name(&pdev->dev), data->device_name))

>>>

>>> I don't fancy the reliance on node names in devicetree, is this well

>>> defined in the binding?

>>

>> I don't think it is.... So, would it be better to just replace the

>> compatible strings for dra7 remoteprocs to be like ti,dra7-dsp1 /

>> ti,dra7-dsp2 etc.? I think that would clean up the code also quite a bit.

>>

> 

> While it would solve "my" problem I'm not entirely sure about it being

> a proper way to flag that they should have different default firmware.

> 

> One way would be to simply rely on a "firmware-name" property read from

> DeviceTree (this was previously objected to, but we have that for

> several bindings now).


Ok, that should work, and would make things easily customizable also.

-Tero

> 

> Regards,

> Bjorn

> 

>>>

>>>> +			return data->fw_name;

>>>> +	}

>>>> +

>>>> +	return ERR_PTR(-ENOENT);

>>>>    }

>>>>    static int omap_rproc_get_boot_data(struct platform_device *pdev,

>>>> @@ -334,7 +384,8 @@ static int omap_rproc_get_boot_data(struct platform_device *pdev,

>>>>    	int ret;

>>>>    	if (!of_device_is_compatible(np, "ti,omap4-dsp") &&

>>>> -	    !of_device_is_compatible(np, "ti,omap5-dsp"))

>>>> +	    !of_device_is_compatible(np, "ti,omap5-dsp") &&

>>>> +	    !of_device_is_compatible(np, "ti,dra7-dsp"))

>>>>    		return 0;

>>>>    	oproc->boot_data = devm_kzalloc(&pdev->dev, sizeof(*oproc->boot_data),

>>>> @@ -360,18 +411,27 @@ static int omap_rproc_get_boot_data(struct platform_device *pdev,

>>>>    		return -EINVAL;

>>>>    	}

>>>> +	if (of_device_is_compatible(np, "ti,dra7-dsp"))

>>>> +		oproc->boot_data->boot_reg_shift = 10;

>>>

>>> Put this in omap_rproc_dev_data.

>>

>> Yeah.

>>

>>>

>>>> +

>>>>    	return 0;

>>>>    }

>>>>    static int omap_rproc_of_get_internal_memories(struct platform_device *pdev,

>>>>    					       struct rproc *rproc)

>>>>    {

>>>> -	static const char * const mem_names[] = {"l2ram"};

>>>> +	static const char * const ipu_mem_names[] = {"l2ram"};

>>>> +	static const char * const dra7_dsp_mem_names[] = {"l2ram", "l1pram",

>>>> +								"l1dram"};

>>>>    	struct device_node *np = pdev->dev.of_node;

>>>>    	struct omap_rproc *oproc = rproc->priv;

>>>>    	struct device *dev = &pdev->dev;

>>>> +	const char * const *mem_names;

>>>>    	struct resource *res;

>>>>    	int num_mems;

>>>> +	const __be32 *addrp;

>>>> +	u32 l4_offset = 0;

>>>> +	u64 size;

>>>>    	int i;

>>>>    	/* OMAP4 and OMAP5 DSPs do not have support for flat SRAM */

>>>> @@ -379,7 +439,15 @@ static int omap_rproc_of_get_internal_memories(struct platform_device *pdev,

>>>>    	    of_device_is_compatible(np, "ti,omap5-dsp"))

>>>>    		return 0;

>>>> -	num_mems = ARRAY_SIZE(mem_names);

>>>> +	/* DRA7 DSPs have two additional SRAMs at L1 level */

>>>> +	if (of_device_is_compatible(np, "ti,dra7-dsp")) {

>>>> +		mem_names = dra7_dsp_mem_names;

>>>> +		num_mems = ARRAY_SIZE(dra7_dsp_mem_names);

>>>> +	} else {

>>>> +		mem_names = ipu_mem_names;

>>>> +		num_mems = ARRAY_SIZE(ipu_mem_names);

>>>> +	}

>>>> +

>>>>    	oproc->mem = devm_kcalloc(dev, num_mems, sizeof(*oproc->mem),

>>>>    				  GFP_KERNEL);

>>>>    	if (!oproc->mem)

>>>> @@ -395,7 +463,26 @@ static int omap_rproc_of_get_internal_memories(struct platform_device *pdev,

>>>>    			return PTR_ERR(oproc->mem[i].cpu_addr);

>>>>    		}

>>>>    		oproc->mem[i].bus_addr = res->start;

>>>> -		oproc->mem[i].dev_addr = OMAP_RPROC_IPU_L2RAM_DEV_ADDR;

>>>> +

>>>> +		/*

>>>> +		 * The DSPs have the internal memories starting at a fixed

>>>> +		 * offset of 0x800000 from address 0, and this corresponds to

>>>> +		 * L2RAM. The L3 address view has the L2RAM bus address as the

>>>> +		 * starting address for the IP, so the L2RAM memory region needs

>>>> +		 * to be processed first, and the device addresses for each

>>>> +		 * memory region can be computed using the relative offset

>>>> +		 * from this base address.

>>>> +		 */

>>>> +		if (of_device_is_compatible(np, "ti,dra7-dsp") &&

>>>

>>> Please don't use a ternary operator repeatedly, it makes the code hard

>>> to follow.

>>

>> Yeah this parts looks somewhat messy, let me try to fix that.

>>

>> -Tero

>>

>>>

>>> Regards,

>>> Bjorn

>>>

>>>> +		    !strcmp(mem_names[i], "l2ram")) {

>>>> +			addrp = of_get_address(dev->of_node, i, &size, NULL);

>>>> +			l4_offset = of_translate_address(dev->of_node, addrp);

>>>> +		}

>>>> +		oproc->mem[i].dev_addr =

>>>> +			of_device_is_compatible(np, "ti,dra7-dsp") ?

>>>> +				res->start - l4_offset +

>>>> +				OMAP_RPROC_DSP_LOCAL_MEM_OFFSET :

>>>> +				OMAP_RPROC_IPU_L2RAM_DEV_ADDR;

>>>>    		oproc->mem[i].size = resource_size(res);

>>>>    		dev_dbg(dev, "memory %8s: bus addr %pa size 0x%x va %p da 0x%x\n",

>>>> -- 

>>>> 2.17.1

>>>>

>>>> --

>>

>> --


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki