mbox series

[V2,00/13] Add multipd remoteproc support

Message ID 20230521222852.5740-1-quic_mmanikan@quicinc.com
Headers show
Series Add multipd remoteproc support | expand

Message

Manikanta Mylavarapu May 21, 2023, 10:28 p.m. UTC
APSS brings Q6 out of reset and then Q6 brings
WCSS block (wifi radio's) out of reset.

				   ---------------
			      -->  |WiFi 2G radio|
			      |	   --------------
			      |
--------	-------	      |
| APSS | --->   |QDSP6|  -----|
---------	-------       |
                              |
      			      |
			      |   --------------
			      --> |WiFi 5G radio|
				  --------------

Problem here is if any radio crashes, subsequently other
radio also should crash because Q6 crashed. Let's say
2G radio crashed, Q6 should pass this info to APSS. Only
Q6 processor interrupts registered with APSS. Obviously
Q6 should crash and raise fatal interrupt to APSS. Due
to this 5G radio also crashed. But no issue in 5G radio,
because of 2G radio crash 5G radio also impacted.

In multi pd model, this problem is resolved. Here WCSS
functionality (WiFi radio's) moved out from Q6 root pd
to a separate user pd. Due to this, radio's independently
pass their status info to APPS with out crashing Q6. So
other radio's won't be impacted.

Pd means protection domain. It's similar to process in Linux.
Here QDSP6 processor runs each wifi radio functionality on a
separate process. One process can't access other process
resources, so this is termed as PD i.e protection domain.

						---------
					    	|WiFi    |
					    --> |2G radio|
					    | 	---------
------	Start Q6     		-------     |
|    |	------------------>     |     |     |
|    |  Start WCSS PD1 (2G)   	|     |	    |
|APSS|	----------------------->|QDSP6|-----|
|    |	Start WCSS PD1 (5G)	|     |
|    |	----------------------->|     |-----|
------		     		-------     |
					    |
					    |	-----------
					    |-->|WiFi	  |
						|5G radio |
						-----------
According to linux terminology, here consider Q6 as root
i.e it provide all services, WCSS (wifi radio's) as user
i.e it uses services provided by root.

Since Q6 root & WCSS user pd's able to communicate with
APSS individually, multipd remoteproc driver registers
each PD with rproc framework. Here clients (Wifi host drivers)
intrested on WCSS PD rproc, so multipd driver start's root
pd in the context of WCSS user pd rproc start. Similarly
on down path, root pd will be stopped after wcss user pd
stopped.

Here WCSS(user) PD is dependent on Q6(root) PD, so first
q6 pd should be up before wcss pd. After wcss pd goes down,
q6 pd should be turned off.

rproc->ops->start(userpd_rproc) {
	/* Boot root pd rproc */
	rproc_boot(upd_dev->parent);
	---
	/* user pd rproc start sequence */
	---
	---
}
With this way we ensure that root pd brought up before userpd.

rproc->ops->stop(userpd_rproc) {
	---
	---
	/* user pd rproc stop sequence */
	---
	---
	/* Shutdown root pd rproc */
	rproc_shutdown(upd_dev->parent);
}
After userpd rproc stops, root pd rproc will be stopped.
IPQ5018, IPQ9574 supports multipd remoteproc driver.

DTS patch [13/13] is based on below series
https://lore.kernel.org/linux-arm-msm/20230519090219.15925-1-quic_devipriy@quicinc.com/
	
[V2]
	- This patch is depends on the below series
https://lore.kernel.org/linux-arm-msm/20230519090219.15925-1-quic_devipriy@quicinc.com/
https://lore.kernel.org/linux-arm-msm/20230519125409.497439-1-quic_srichara@quicinc.com/
	- Fixed all comments and rebased for TOT.
	- Strip off SCM code changes from https://lore.kernel.org/linux-arm-msm/1678164097-13247-9-git-send-email-quic_mmanikan@quicinc.com/
	  and made new patchset [08/13]
	- Strip off qcom_q6v5 changes from https://lore.kernel.org/linux-arm-msm/1678164097-13247-9-git-send-email-quic_mmanikan@quicinc.com/
	  and made new patchset [09/13]
	- since clocks handled by QDSP6 firmware
	  Dropped https://lore.kernel.org/linux-arm-msm/1678164097-13247-6-git-send-email-quic_mmanikan@quicinc.com/,
	  https://lore.kernel.org/linux-arm-msm/1678164097-13247-7-git-send-email-quic_mmanikan@quicinc.com/.
 	  Added [04/13], [05/13], [06/13], [07/13] patches.

Manikanta Mylavarapu (13):
  dt-bindings: remoteproc: qcom: Add support for multipd model
  dt-bindings: mailbox: qcom: Add IPQ5018 APCS compatible
  dt-bindings: arm: qcom: Document the Qualcomm rdp432-c1 board
  dt-bindings: clock: qcom: gcc-ipq5018: remove q6 clocks macros
  dt-bindings: clock: qcom: gcc-ipq9574: remove q6 bring up clock macros
  clk: qcom: ipq5018: remove q6 bring up clocks
  clk: qcom: ipq9574: remove q6 bring up clocks
  firmware: qcom_scm: ipq5018: Add WCSS AHB pd support
  remoteproc: qcom: q6v5: Add multipd interrupts support
  remoteproc: qcom: Add Hexagon based multipd rproc driver
  arm64: dtsi: qcom: ipq5018: enable nodes required for multipd
  arm64: dts: qcom: ipq5018: Add RDP432-c1 board support
  arm64: dtsi: qcom: ipq9574: Add nodes to bring up multipd

 .../devicetree/bindings/arm/qcom.yaml         |   1 +
 .../mailbox/qcom,apcs-kpss-global.yaml        |   1 +
 .../bindings/remoteproc/qcom,multipd-pil.yaml | 265 +++++++
 arch/arm64/boot/dts/qcom/Makefile             |   1 +
 .../arm64/boot/dts/qcom/ipq5018-rdp432-c1.dts |  49 ++
 arch/arm64/boot/dts/qcom/ipq5018.dtsi         | 140 ++++
 arch/arm64/boot/dts/qcom/ipq9574.dtsi         | 118 +++
 drivers/clk/qcom/gcc-ipq5018.c                | 414 -----------
 drivers/clk/qcom/gcc-ipq9574.c                | 326 ---------
 drivers/firmware/qcom_scm.c                   | 114 +++
 drivers/firmware/qcom_scm.h                   |   6 +
 drivers/remoteproc/Kconfig                    |  20 +
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/qcom_common.h              |   9 +
 drivers/remoteproc/qcom_q6v5.c                |  37 +-
 drivers/remoteproc/qcom_q6v5.h                |  11 +
 drivers/remoteproc/qcom_q6v5_mpd.c            | 677 ++++++++++++++++++
 drivers/soc/qcom/mdt_loader.c                 | 332 +++++++++
 include/dt-bindings/clock/qcom,gcc-ipq5018.h  |  21 -
 include/dt-bindings/clock/qcom,ipq9574-gcc.h  |  18 -
 include/linux/firmware/qcom/qcom_scm.h        |   3 +
 include/linux/soc/qcom/mdt_loader.h           |  19 +
 22 files changed, 1801 insertions(+), 782 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
 create mode 100644 arch/arm64/boot/dts/qcom/ipq5018-rdp432-c1.dts
 create mode 100644 drivers/remoteproc/qcom_q6v5_mpd.c


base-commit: dbd91ef4e91c1ce3a24429f5fb3876b7a0306733
--
2.17.1

Comments

Manikanta Mylavarapu June 5, 2023, 5:38 a.m. UTC | #1
On 6/5/2023 11:05 AM, Manikanta Mylavarapu wrote:
> 
> 
> On 5/30/2023 4:35 PM, Krzysztof Kozlowski wrote:
>> On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
>>> Enable nodes required for multipd remoteproc bring up
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
>>
>> It is "dts". Not "dtsi".
>>
> Sure, I will update to 'dtsi'.
> 
Sorry please discard my previous reply.
I will update to dts.

>>>
>>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>>> ---
>>> Changes in V2:
>>>     - Corrected syntax like alignmnet and kept nodes in sorted order.
>>>     - Covered entire TCSR region.
>>>     - Added 'firmware-name' property.
>>>
>>>   arch/arm64/boot/dts/qcom/ipq5018.dtsi | 140 ++++++++++++++++++++++++++
>>>   1 file changed, 140 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi 
>>> b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>>> index 9f13d2dcdfd5..3772d54d89e4 100644
>>> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>>> @@ -85,6 +85,18 @@
>>>               reg = <0x0 0x4ac00000 0x0 0x200000>;
>>>               no-map;
>>>           };
>>> +
>>> +        q6_region: wcnss@4b000000 {
>>> +            reg = <0x0 0x4b000000 0x0 0x1700000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        smem@4ab00000 {
>>> +            compatible = "qcom,smem";
>>> +            reg = <0x0 0x4ab00000 0x0 0x100000>;
>>> +            hwlocks = <&tcsr_mutex 0>;
>>> +            no-map;
>>> +        };
>>>       };
>>>
>>>       soc: soc@0 {
>>> @@ -128,6 +140,12 @@
>>>               #power-domain-cells = <1>;
>>>           };
>>>
>>> +        tcsr_mutex: hwlock@1905000 {
>>> +            compatible = "qcom,tcsr-mutex";
>>> +            reg = <0x01905000 0x20000>;
>>> +            #hwlock-cells = <1>;
>>> +        };
>>> +
>>>           sdhc_1: mmc@7804000 {
>>>               compatible = "qcom,ipq5018-sdhci", "qcom,sdhci-msm-v5";
>>>               reg = <0x7804000 0x1000>;
>>> @@ -181,6 +199,14 @@
>>>               };
>>>           };
>>>
>>> +        apcs_glb: mailbox@b111000 {
>>> +            compatible = "qcom,ipq5018-apcs-apps-global",
>>> +                     "qcom,ipq6018-apcs-apps-global";
>>> +            reg = <0x0b111000 0x1000>;
>>> +            #clock-cells = <1>;
>>> +            #mbox-cells = <1>;
>>> +        };
>>> +
>>>           timer@b120000 {
>>>               compatible = "arm,armv7-timer-mem";
>>>               reg = <0x0b120000 0x1000>;
>>> @@ -238,6 +264,96 @@
>>>                   status = "disabled";
>>>               };
>>>           };
>>> +
>>> +        q6v5_wcss: remoteproc@cd00000 {
>>> +            compatible = "qcom,ipq5018-q6-mpd";
>>> +            reg = <0x0cd00000 0x4040>;
>>> +            firmware-name = "IPQ5018/q6_fw.mdt",
>>> +                    "IPQ5018/m3_fw.mdt",
>>> +                    "qcn6122/m3_fw.mdt";
>>> +            interrupts-extended = <&intc GIC_SPI 291 
>>> IRQ_TYPE_EDGE_RISING>,
>>> +                          <&wcss_smp2p_in 0 0>,
>>> +                          <&wcss_smp2p_in 1 0>,
>>> +                          <&wcss_smp2p_in 2 0>,
>>> +                          <&wcss_smp2p_in 3 0>;
>>> +            interrupt-names = "wdog",
>>> +                      "fatal",
>>> +                      "ready",
>>> +                      "handover",
>>> +                      "stop-ack";
>>> +
>>> +            qcom,smem-states = <&wcss_smp2p_out 0>,
>>> +                       <&wcss_smp2p_out 1>;
>>> +            qcom,smem-state-names = "shutdown",
>>> +                        "stop";
>>> +            memory-region = <&q6_region>;
>>> +
>>> +            glink-edge {
>>> +                interrupts = <GIC_SPI 179 IRQ_TYPE_EDGE_RISING>;
>>> +                label = "rtr";
>>> +                qcom,remote-pid = <1>;
>>> +                mboxes = <&apcs_glb 8>;
>>> +            };
>>> +
>>> +            pd-1 {
>>> +                compatible = "qcom,ipq5018-wcss-ahb-mpd";
>>> +                firmware-name = "IPQ5018/q6_fw.mdt";
>>> +                interrupts-extended = <&wcss_smp2p_in 8 0>,
>>> +                              <&wcss_smp2p_in 9 0>,
>>> +                              <&wcss_smp2p_in 12 0>,
>>> +                              <&wcss_smp2p_in 11 0>;
>>
>> What "0" stands for?
>>
> 0 means IRQ_NONE. These are software interrupts (register write),
> so we have configured to 0. I will replace 0 with IRQ_NONE.
> 
> Thanks & Regards,
> Manikanta.
> 
>>> +                interrupt-names = "fatal",
>>> +                          "ready",
>> Best regards,
>> Krzysztof
>>
Krzysztof Kozlowski June 14, 2023, 1:59 p.m. UTC | #2
On 14/06/2023 13:43, Manikanta Mylavarapu wrote:
>>>>>>>>> +    properties:
>>>>>>>>> +      compatible:
>>>>>>>>> +        enum:
>>>>>>>>> +          - qcom,ipq5018-wcss-ahb-mpd
>>>>>>>>> +          - qcom,ipq9574-wcss-ahb-mpd
>>>>>>>>> +          - qcom,ipq5018-wcss-pcie-mpd
>>>>>>>>
>>>>>>>> Keep rather alphabetical order (so both 5018 together).
>>>>>>>>
>>>>>>>> I also do not understand these at all. Why adding bus type to
>>>>>>>> compatible? This rarely is allowed (unless it is PCIe controller within
>>>>>>>> soc).
>>>>>>>>
>>>>>>> IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
>>>>>>> external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE
>>>>>>> radio's properties, i have added bus type to compatible.
>>>>>>
>>>>>> It's the same device - WCSS - right? We do not create multiple nodes and
>>>>>> compatibles for the same devices. Bus suffixes are almost never parts of
>>>>>> compatibles.
>>>>>
>>>>>
>>>>> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached
>>>>> via pcie to IPQ5018. Here QDSP6 managing both WCSS's.
>>>>>
>>>>> So for better clarity i will use attached SOC ID in compatible.
>>>>> Below are the new compatible's.
>>>>>
>>>>> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio
>>>>> - qcom,ipq9574-wcss-mpd	//IPQ9574 internal radio
>>>>> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio
>>>>
>>>> What mandates that there's just one QCN6122 device attached to PCI?
>>>> Assuming fixed PCI configurations like that makes me worried.
>>>>
>>>
>>> IPQ5018 always has one internal radio, attached pcie radio's depends on
>>> no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two
>>> qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's
>>> number of pcie devices controlled by QDSP6.
>>
>> So this is hot-pluggable (or at least board-pluggable), then should not
>> be a part of static DTS.
>>
>> Some concepts of virtual-processes is anyway far away from hardware
>> description, thus does not fit into DTS. Adding now to the equation PCIe
>> with variable number of such processes, brings us even further.
>>
>> This is not a DT property. Remember - DT describes hardware.
>>
>> Best regards,
>> Krzysztof
>>
> 
> In the multipd architecture based Socs, There is one Q6 DSP which runs 
> the OS/kernel and there are one or more instances of WCSS radios
> (It can be either internal or pcie attached).
> These WCSS cores are controlled by the Q6 (Q6 DSP brings wcss radios out 
> of reset/ shuts it down etc). Q6 forms the 'root Protection domain' and 
> the wcss radios are termed as the 'user protection domain'.
> The compatible's that is being added here are to manage the 'root 
> domain' and 'user domain'.
> Not sure if using the words 'pcie'/'ahb' made it confusing.
> So, 'qcom,ipq5018-q6-mpd' and 'qcom,ipq5018-wcss-mpd'.
> 
> There will be multiple instances of 'qcom,ipq5018-wcss-mpd' in DT based 
> on number of wcss radios connected on that board and only one instance 
> of 'qcom,ipq5018-q6-mpd'.
> 

I don't understand why the user protection domains need a specific
compatible. Why do they need compatible at all?

Not mentioning that amount of your domains on Q6 is actually fixed per
SoC and probably should not be in DT at all.

Qualcomm puts so many so weird stuff into DT which is not a hardware
description. I understand that everything is there a firmware, but then
make it discoverable for example...

Best regards,
Krzysztof
Krzysztof Kozlowski June 24, 2023, 7:01 a.m. UTC | #3
On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
> Enable nodes required for multipd remoteproc bring up.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> ---
> Changes in V2:
> 	- Corrected syntax like alignmnet and kept nodes in sorted order.
> 	- Added 'firmware-name' property.
> 
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 118 ++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index 0e04549c69a5..ff0da53ba05f 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -160,6 +160,11 @@
>  			no-map;
>  		};
> 
> +		q6_region: wcnss@4ab00000 {
> +			reg = <0x0 0x4ab00000 0x0 0x2b00000>;
> +			no-map;
> +		};
> +
>  		smem@4aa00000 {
>  			compatible = "qcom,smem";
>  			reg = <0x0 0x4aa00000 0x0 0x00100000>;
> @@ -697,6 +702,95 @@
>  			};
>  		};
> 
> +		q6v5_wcss: remoteproc@cd00000 {
> +			compatible = "qcom,ipq9574-q6-mpd";
> +			reg = <0x0cd00000 0x4040>;
> +			firmware-name = "IPQ9574/q6_fw.mdt",
> +					"IPQ9574/m3_fw.mdt";

Here and...

> +			interrupts-extended = <&intc GIC_SPI 325 IRQ_TYPE_EDGE_RISING>,
> +					      <&wcss_smp2p_in 0 0>,
> +					      <&wcss_smp2p_in 1 0>,
> +					      <&wcss_smp2p_in 2 0>,
> +					      <&wcss_smp2p_in 3 0>;
> +			interrupt-names = "wdog",
> +					  "fatal",
> +					  "ready",
> +					  "handover",
> +					  "stop-ack";
> +
> +			qcom,smem-states = <&wcss_smp2p_out 0>,
> +					   <&wcss_smp2p_out 1>;
> +			qcom,smem-state-names = "shutdown",
> +						"stop";
> +			memory-region = <&q6_region>;
> +
> +			glink-edge {
> +				interrupts = <GIC_SPI 321 IRQ_TYPE_EDGE_RISING>;
> +				label = "rtr";
> +				qcom,remote-pid = <1>;
> +				mboxes = <&apcs_glb 8>;
> +			};
> +
> +			pd-1 {
> +				compatible = "qcom,ipq9574-wcss-ahb-mpd";
> +				firmware-name = "IPQ9574/q6_fw.mdt";

... here - why do you have firmware in both places?

> +				interrupts-extended = <&wcss_smp2p_in 8 0>,
> +						      <&wcss_smp2p_in 9 0>,
> +						      <&wcss_smp2p_in 12 0>,
> +						      <&wcss_smp2p_in 11 0>;
> +				interrupt-names = "fatal",
> +						  "ready",
> +						  "spawn-ack",
> +						  "stop-ack";
> +				qcom,smem-states = <&wcss_smp2p_out 8>,
> +						   <&wcss_smp2p_out 9>,
> +						   <&wcss_smp2p_out 10>;
> +				qcom,smem-state-names = "shutdown",
> +							"stop",
> +							"spawn";
> +			};
> +
> +			pd-2 {
> +				compatible = "qcom,ipq5018-wcss-pcie-mpd";

This compatible is confusing for this device.

Best regards,
Krzysztof
Krzysztof Kozlowski June 24, 2023, 7:19 a.m. UTC | #4
On 21/06/2023 13:39, Manikanta Mylavarapu wrote:
>>> on number of wcss radios connected on that board and only one instance
>>> of 'qcom,ipq5018-q6-mpd'.
>>>
>>
>> I don't understand why the user protection domains need a specific
>> compatible. Why do they need compatible at all?
>>
>> Not mentioning that amount of your domains on Q6 is actually fixed per
>> SoC and probably should not be in DT at all.
>>
>    root domain is fixed per soc (One Q6 DSP, one per soc)
>    user domain(s) are variable (based on number of wcss radios attached)
> 
>    The sequence to initialize, bring up, tear down the Q6 and wcss radios
>    are completely different. So in order to differentiate them, we will
>    need different compatibles. So this is a new rproc driver/architecture
>    which has a parent/child topology (Q6 DSP -> Master/parent controls
>    the WCSS (child)).

I understand you need different properties, but I don't see yet the
benefit of creating here artificial compatibles. Look at your ipq9574
DTSI change - it does not use even ipq9574 compatibles for 66% of its
children.

Maybe you should have there just property describing type of device or
bringup?

Given SoC cannot come with different amount of children (PD) and
different amount of radios. You even fix the firmware name, so
boards/customers cannot use anything else. It's fixed and the only
variable element here is disabling some of the blocks per board, if they
do not have physical interface (e.g. radio).

You even hard-code the number of PD by using "pd-[123]", without unit
address, so you do not expect it will grow.

Unless you want to say that these are devices? But your binding does not
really suggest it...


Best regards,
Krzysztof
Krzysztof Kozlowski June 24, 2023, 7:28 a.m. UTC | #5
On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
> It adds support to bring up remoteproc's on multipd model.
> Pd means protection domain. It's similar to process in Linux.
> Here QDSP6 processor runs each wifi radio functionality on a
> separate process. One process can't access other process
> resources, so this is termed as PD i.e protection domain.
> 
> Here we have two pd's called root and user pd. We can correlate
> Root pd as root and user pd as user in linux. Root pd has more
> privileges than user pd. Root will provide services to user pd.
> 
> From remoteproc driver perspective, root pd corresponds to QDSP6
> processor bring up and user pd corresponds to Wifi radio (WCSS)
> bring up.
> 
> Here WCSS(user) PD is dependent on Q6(root) PD, so first
> q6 pd should be up before wcss pd. After wcss pd goes down,
> q6 pd should be turned off.
> 
> IPQ5018, IPQ9574 supports multipd remoteproc driver.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> ---
> Changes in V2:
> 	- Common functionalities moved to seperate patches
> 	- qcom_get_pd_asid() moved to mpd driver
> 	- Last DMA block alone memset to zero
> 	- Added diagram to show how userpd data is organized and sent to
> 	  trustzone
> 	- Rewritten commit message since most of the content available
> 	  in cover page
> 	- Removed 'remote_id' becuase it's not required for bring up.
> 
>  drivers/remoteproc/Kconfig          |  20 +
>  drivers/remoteproc/Makefile         |   1 +
>  drivers/remoteproc/qcom_common.h    |   9 +
>  drivers/remoteproc/qcom_q6v5_mpd.c  | 677 ++++++++++++++++++++++++++++
>  drivers/soc/qcom/mdt_loader.c       | 332 ++++++++++++++
>  include/linux/soc/qcom/mdt_loader.h |  19 +
>  6 files changed, 1058 insertions(+)
>  create mode 100644 drivers/remoteproc/qcom_q6v5_mpd.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index a850e9f486dd..44af5c36f67e 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -234,6 +234,26 @@ config QCOM_Q6V5_PAS
>  	  CDSP (Compute DSP), MPSS (Modem Peripheral SubSystem), and
>  	  SLPI (Sensor Low Power Island).
> 
> +config QCOM_Q6V5_MPD
> +	tristate "Qualcomm Hexagon based MPD model Peripheral Image Loader"
> +	depends on OF && ARCH_QCOM
> +	depends on QCOM_SMEM
> +	depends on RPMSG_QCOM_SMD || RPMSG_QCOM_SMD=n
> +	depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
> +	depends on QCOM_SYSMON || QCOM_SYSMON=n
> +	depends on RPMSG_QCOM_GLINK || RPMSG_QCOM_GLINK=n
> +	depends on QCOM_AOSS_QMP || QCOM_AOSS_QMP=n
> +	select MFD_SYSCON

Do you really need this?

> +	select QCOM_MDT_LOADER
> +	select QCOM_PIL_INFO
> +	select QCOM_Q6V5_COMMON
> +	select QCOM_RPROC_COMMON
> +	select QCOM_SCM
> +	help
> +	  Say y here to support the Qualcomm Secure Peripheral Image Loader
> +	  for the Hexagon based MultiPD model remote processors on e.g. IPQ5018.
> +	  This is trustZone wireless subsystem.
> +

...

> +	int (*powerup_scm)(u32 peripheral);
> +	int (*powerdown_scm)(u32 peripheral);
> +};
> +
> +/**
> + * qcom_get_pd_asid() - get the pd asid number from DT node
Manikanta Mylavarapu June 30, 2023, 7:11 a.m. UTC | #6
On 6/27/2023 1:14 PM, Manikanta Mylavarapu wrote:
> 
> 
> On 6/24/2023 12:31 PM, Krzysztof Kozlowski wrote:
>> On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
>>> Enable nodes required for multipd remoteproc bring up.
>>>
>>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>>> ---
>>> Changes in V2:
>>>     - Corrected syntax like alignmnet and kept nodes in sorted order.
>>>     - Added 'firmware-name' property.
>>>
>>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi | 118 ++++++++++++++++++++++++++
>>>   1 file changed, 118 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi 
>>> b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>> index 0e04549c69a5..ff0da53ba05f 100644
>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>> @@ -160,6 +160,11 @@
>>>               no-map;
>>>           };
>>>
>>> +        q6_region: wcnss@4ab00000 {
>>> +            reg = <0x0 0x4ab00000 0x0 0x2b00000>;
>>> +            no-map;
>>> +        };
>>> +
>>>           smem@4aa00000 {
>>>               compatible = "qcom,smem";
>>>               reg = <0x0 0x4aa00000 0x0 0x00100000>;
>>> @@ -697,6 +702,95 @@
>>>               };
>>>           };
>>>
>>> +        q6v5_wcss: remoteproc@cd00000 {
>>> +            compatible = "qcom,ipq9574-q6-mpd";
>>> +            reg = <0x0cd00000 0x4040>;
>>> +            firmware-name = "IPQ9574/q6_fw.mdt",
>>> +                    "IPQ9574/m3_fw.mdt";
>>
>> Here and...
>>
>>> +            interrupts-extended = <&intc GIC_SPI 325 
>>> IRQ_TYPE_EDGE_RISING>,
>>> +                          <&wcss_smp2p_in 0 0>,
>>> +                          <&wcss_smp2p_in 1 0>,
>>> +                          <&wcss_smp2p_in 2 0>,
>>> +                          <&wcss_smp2p_in 3 0>;
>>> +            interrupt-names = "wdog",
>>> +                      "fatal",
>>> +                      "ready",
>>> +                      "handover",
>>> +                      "stop-ack";
>>> +
>>> +            qcom,smem-states = <&wcss_smp2p_out 0>,
>>> +                       <&wcss_smp2p_out 1>;
>>> +            qcom,smem-state-names = "shutdown",
>>> +                        "stop";
>>> +            memory-region = <&q6_region>;
>>> +
>>> +            glink-edge {
>>> +                interrupts = <GIC_SPI 321 IRQ_TYPE_EDGE_RISING>;
>>> +                label = "rtr";
>>> +                qcom,remote-pid = <1>;
>>> +                mboxes = <&apcs_glb 8>;
>>> +            };
>>> +
>>> +            pd-1 {
>>> +                compatible = "qcom,ipq9574-wcss-ahb-mpd";
>>> +                firmware-name = "IPQ9574/q6_fw.mdt";
>>
>> ... here - why do you have firmware in both places?
>>

	In multipd model, Q6 & WCSS uses different firmware.
	I will correct the firmware-name. Thanks for catching.

>>> +                interrupts-extended = <&wcss_smp2p_in 8 0>,
>>> +                              <&wcss_smp2p_in 9 0>,
>>> +                              <&wcss_smp2p_in 12 0>,
>>> +                              <&wcss_smp2p_in 11 0>;
>>> +                interrupt-names = "fatal",
>>> +                          "ready",
>>> +                          "spawn-ack",
>>> +                          "stop-ack";
>>> +                qcom,smem-states = <&wcss_smp2p_out 8>,
>>> +                           <&wcss_smp2p_out 9>,
>>> +                           <&wcss_smp2p_out 10>;
>>> +                qcom,smem-state-names = "shutdown",
>>> +                            "stop",
>>> +                            "spawn";
>>> +            };
>>> +
>>> +            pd-2 {
>>> +                compatible = "qcom,ipq5018-wcss-pcie-mpd";
>>
>> This compatible is confusing for this device.
>>
	I will clean up all SOC specific compatibles and have
	only device specific compatibles for Q6 & WCSS radio's
	as i mentioned on other thread.

Thanks & Regards,
Manikanta.
Manikanta Mylavarapu June 30, 2023, 10:29 a.m. UTC | #7
On 6/27/2023 6:09 PM, Manikanta Mylavarapu wrote:
> 
> 
> On 6/24/2023 12:58 PM, Krzysztof Kozlowski wrote:
>> On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
>>> It adds support to bring up remoteproc's on multipd model.
>>> Pd means protection domain. It's similar to process in Linux.
>>> Here QDSP6 processor runs each wifi radio functionality on a
>>> separate process. One process can't access other process
>>> resources, so this is termed as PD i.e protection domain.
>>>
>>> Here we have two pd's called root and user pd. We can correlate
>>> Root pd as root and user pd as user in linux. Root pd has more
>>> privileges than user pd. Root will provide services to user pd.
>>>
>>>  From remoteproc driver perspective, root pd corresponds to QDSP6
>>> processor bring up and user pd corresponds to Wifi radio (WCSS)
>>> bring up.
>>>
>>> Here WCSS(user) PD is dependent on Q6(root) PD, so first
>>> q6 pd should be up before wcss pd. After wcss pd goes down,
>>> q6 pd should be turned off.
>>>
>>> IPQ5018, IPQ9574 supports multipd remoteproc driver.
>>>
>>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>>> ---
>>> Changes in V2:
>>>     - Common functionalities moved to seperate patches
>>>     - qcom_get_pd_asid() moved to mpd driver
>>>     - Last DMA block alone memset to zero
>>>     - Added diagram to show how userpd data is organized and sent to
>>>       trustzone
>>>     - Rewritten commit message since most of the content available
>>>       in cover page
>>>     - Removed 'remote_id' becuase it's not required for bring up.
>>>
>>>   drivers/remoteproc/Kconfig          |  20 +
>>>   drivers/remoteproc/Makefile         |   1 +
>>>   drivers/remoteproc/qcom_common.h    |   9 +
>>>   drivers/remoteproc/qcom_q6v5_mpd.c  | 677 ++++++++++++++++++++++++++++
>>>   drivers/soc/qcom/mdt_loader.c       | 332 ++++++++++++++
>>>   include/linux/soc/qcom/mdt_loader.h |  19 +
>>>   6 files changed, 1058 insertions(+)
>>>   create mode 100644 drivers/remoteproc/qcom_q6v5_mpd.c
>>>
>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>> index a850e9f486dd..44af5c36f67e 100644
>>> --- a/drivers/remoteproc/Kconfig
>>> +++ b/drivers/remoteproc/Kconfig
>>> @@ -234,6 +234,26 @@ config QCOM_Q6V5_PAS
>>>         CDSP (Compute DSP), MPSS (Modem Peripheral SubSystem), and
>>>         SLPI (Sensor Low Power Island).
>>>
>>> +config QCOM_Q6V5_MPD
>>> +    tristate "Qualcomm Hexagon based MPD model Peripheral Image Loader"
>>> +    depends on OF && ARCH_QCOM
>>> +    depends on QCOM_SMEM
>>> +    depends on RPMSG_QCOM_SMD || RPMSG_QCOM_SMD=n
>>> +    depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
>>> +    depends on QCOM_SYSMON || QCOM_SYSMON=n
>>> +    depends on RPMSG_QCOM_GLINK || RPMSG_QCOM_GLINK=n
>>> +    depends on QCOM_AOSS_QMP || QCOM_AOSS_QMP=n
>>> +    select MFD_SYSCON
>>
>> Do you really need this?
>>
	It's not required. I will remove. Thanks for catching.

>>> +    select QCOM_MDT_LOADER
>>> +    select QCOM_PIL_INFO
>>> +    select QCOM_Q6V5_COMMON
>>> +    select QCOM_RPROC_COMMON
>>> +    select QCOM_SCM
>>> +    help
>>> +      Say y here to support the Qualcomm Secure Peripheral Image Loader
>>> +      for the Hexagon based MultiPD model remote processors on e.g. 
>>> IPQ5018.
>>> +      This is trustZone wireless subsystem.
>>> +
>>
>> ...
>>
	I didn't understand. Can you please elaborate your comment?

>>> +    int (*powerup_scm)(u32 peripheral);
>>> +    int (*powerdown_scm)(u32 peripheral);
>>> +};
>>> +
>>> +/**
>>> + * qcom_get_pd_asid() - get the pd asid number from DT node
>>
>>  From node name? NAK. It does not work like that. Node names can change
>> and you did not define this number as part of ABI.
>>
>> Probably you wanted unit address.
>>

	Yeah i got your point. Each of the WCSS PD's are internally
	represented in Q6 with their corresponding "spawn" bit numbers.
	I will use same and remove the "PD-" hardcodings.

	Is this fine ?

>>> + * @node:    device tree node
>>> + *
>>> + * Returns asid if node name has 'pd' string
>>> + */
>>> +s8 qcom_get_pd_asid(struct device_node *node)
>>> +{
>>> +    char *str;
>>> +    s8 pd_asid;
>>> +
>>> +    if (!node)
>>> +        return -EINVAL;
>>> +
>>> +    str = strstr(node->name, "pd");
>>> +    if (!str)
>>> +        return 0;
>>> +
>>> +    str += strlen("pd") + 1;
>>> +    return kstrtos8(str, 10, &pd_asid) ? -EINVAL : pd_asid;
>>> +}
>>> +EXPORT_SYMBOL(qcom_get_pd_asid);
>>> +
>>
>> ...
>>
	
	I didn't understand. Can you please elaborate your comment?

>>> +
>>> +static int q6_wcss_spawn_pd(struct rproc *rproc)
>>> +{
>>> +    int ret;
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +
>>> +    ret = qcom_q6v5_request_spawn(&wcss->q6);
>>> +    if (ret == -ETIMEDOUT) {
>>> +        pr_err("%s spawn timedout\n", rproc->name);
>>
>> dev_err
>>

	Sure, I will change to dev_err.

>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = qcom_q6v5_wait_for_start(&wcss->q6, msecs_to_jiffies(10000));
>>> +    if (ret == -ETIMEDOUT) {
>>> +        pr_err("%s start timedout\n", rproc->name);
>>
>> dev_err
>>

	Sure, I will change to dev_err.

>>> +        wcss->q6.running = false;
>>> +        return ret;
>>> +    }
>>> +    wcss->q6.running = true;
>>> +    return ret;
>>> +}
>>> +
>>> +static int wcss_ahb_pcie_pd_start(struct rproc *rproc)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +    const struct wcss_data *desc = wcss->desc;
>>> +    int ret;
>>> +
>>> +    if (!desc->reset_seq)
>>> +        return 0;
>>> +
>>> +    if (desc->powerup_scm) {
>>> +        ret = desc->powerup_scm(desc->pasid);
>>> +        if (ret) {
>>> +            dev_err(wcss->dev, "failed to power up pd\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    ret = q6_wcss_spawn_pd(rproc);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    wcss->state = WCSS_NORMAL;
>>> +    return ret;
>>> +}
>>> +
>>> +static int q6_wcss_stop(struct rproc *rproc)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +    const struct wcss_data *desc = wcss->desc;
>>> +    int ret;
>>> +
>>> +    ret = qcom_scm_pas_shutdown(desc->pasid);
>>> +    if (ret) {
>>> +        dev_err(wcss->dev, "not able to shutdown\n");
>>> +        return ret;
>>> +    }
>>> +    qcom_q6v5_unprepare(&wcss->q6);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int wcss_ahb_pcie_pd_stop(struct rproc *rproc)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +    struct rproc *rpd_rproc = dev_get_drvdata(wcss->dev->parent);
>>> +    const struct wcss_data *desc = wcss->desc;
>>> +    int ret;
>>> +
>>> +    if (!desc->reset_seq)
>>> +        goto shut_down_rpd;
>>> +
>>> +    if (rproc->state != RPROC_CRASHED && wcss->q6.stop_bit) {
>>> +        ret = qcom_q6v5_request_stop(&wcss->q6, NULL);
>>> +        if (ret) {
>>> +            dev_err(&rproc->dev, "pd not stopped\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    if (desc->powerdown_scm) {
>>> +        ret = desc->powerdown_scm(desc->pasid);
>>> +        if (ret) {
>>> +            dev_err(wcss->dev, "failed to power down pd\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +shut_down_rpd:
>>> +    rproc_shutdown(rpd_rproc);
>>> +
>>> +    wcss->state = WCSS_SHUTDOWN;
>>> +    return 0;
>>> +}
>>> +
>>> +static void *q6_wcss_da_to_va(struct rproc *rproc, u64 da, size_t len,
>>> +                  bool *is_iomem)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +    int offset;
>>> +
>>> +    offset = da - wcss->mem_reloc;
>>> +    if (offset < 0 || offset + len > wcss->mem_size)
>>> +        return NULL;
>>> +
>>> +    return wcss->mem_region + offset;
>>> +}
>>> +
>>> +static int q6_wcss_load(struct rproc *rproc, const struct firmware *fw)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +    const struct firmware *fw_hdl;
>>> +    int ret;
>>> +    const struct wcss_data *desc = wcss->desc;
>>> +    int loop;
>>> +
>>> +    ret = qcom_mdt_load(wcss->dev, fw, rproc->firmware,
>>> +                desc->pasid, wcss->mem_region,
>>> +                wcss->mem_phys, wcss->mem_size,
>>> +                &wcss->mem_reloc);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    for (loop = 1; loop < MAX_FIRMWARE; loop++) {
>>> +        if (!wcss->firmware[loop])
>>> +            continue;
>>> +
>>> +        ret = request_firmware(&fw_hdl, wcss->firmware[loop],
>>> +                       wcss->dev);
>>> +        if (ret)
>>> +            continue;
>>> +
>>> +        ret = qcom_mdt_load_no_init(wcss->dev, fw_hdl,
>>> +                        wcss->firmware[loop], 0,
>>> +                        wcss->mem_region,
>>> +                        wcss->mem_phys,
>>> +                        wcss->mem_size,
>>> +                        &wcss->mem_reloc);
>>> +
>>> +        release_firmware(fw_hdl);
>>> +
>>> +        if (ret) {
>>> +            dev_err(wcss->dev,
>>> +                "can't load %s ret:%d\n", wcss->firmware[loop], ret);
>>> +            return ret;
>>> +        }
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +/* This function load's userpd firmware. Since Userpd depends on rootpd
>>
>> Use Linux coding style comments.
>>

	Sure, I will update.

>>> + * first bring up root pd and then load. User pd firmware load is 
>>> required
>>> + * only during user pd restart because root pd loads user pd FW pil 
>>> segments
>>> + * during it's bringup.
>>> + */
>>> +static int wcss_ahb_pcie_pd_load(struct rproc *rproc, const struct 
>>> firmware *fw)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv, *wcss_rpd;
>>> +    struct rproc *rpd_rproc = dev_get_drvdata(wcss->dev->parent);
>>> +    const struct wcss_data *desc = wcss->desc;
>>> +    int ret;
>>> +
>>> +    wcss_rpd = rpd_rproc->priv;
>>> +
>>> +    /* Boot rootpd rproc */
>>
>> That's not helpful - i tcopies the function name.
>>

	sure, i will remove comment.

>>> +    ret = rproc_boot(rpd_rproc);
>>> +    if (ret || wcss->state == WCSS_NORMAL)
>>> +        return ret;
>>> +
>>> +    return desc->mdt_load_sec(wcss->dev, fw, rproc->firmware,
>>> +                  desc->pasid, wcss->mem_region,
>>> +                  wcss->mem_phys, wcss->mem_size,
>>> +                  &wcss->mem_reloc);
>>> +}
>>> +

...

	I didn't understand. Can you please elaborate your comment?

>>> +static int q6_get_inbound_irq(struct qcom_q6v5 *q6,
>>> +                  struct platform_device *pdev,
>>> +                  const char *int_name,
>>> +                  irqreturn_t (*handler)(int irq, void *data))
>>> +{
>>> +    int ret, irq;
>>> +    char *interrupt, *tmp = (char *)int_name;
>>> +    struct q6_wcss *wcss = q6->rproc->priv;
>>> +
>>> +    irq = platform_get_irq_byname(pdev, int_name);
>>> +    if (irq < 0) {
>>> +        if (irq != -EPROBE_DEFER)
>>> +            dev_err(&pdev->dev,
>>> +                "failed to retrieve %s IRQ: %d\n",
>>> +                    int_name, irq);
>>
>> return dev_err_probe
>>

	I will use dev_err_probe.

>>> +        return irq;
>>> +    }
>>> +
>>> +    if (!strcmp(int_name, "fatal")) {
>>> +        q6->fatal_irq = irq;
>>> +    } else if (!strcmp(int_name, "stop-ack")) {
>>> +        q6->stop_irq = irq;
>>> +        tmp = "stop_ack";
>>> +    } else if (!strcmp(int_name, "ready")) {
>>> +        q6->ready_irq = irq;
>>> +    } else if (!strcmp(int_name, "handover")) {
>>> +        q6->handover_irq  = irq;
>>> +    } else if (!strcmp(int_name, "spawn-ack")) {
>>> +        q6->spawn_irq = irq;
>>> +        tmp = "spawn_ack";
>>> +    } else {
>>> +        dev_err(&pdev->dev, "unknown interrupt\n");
>>> +        return -EINVAL;
>>> +    }
>>
>> This is over complicated method of getting interrupts. First, you can
>> pass here address of variable with interrupt to assign (*irq_field =
>> irq). Second, drop the names and get by index. Entries are fixed, not
>> flexible.
>>
	Sure, I will do it.

>>> +
>>> +    interrupt = devm_kzalloc(&pdev->dev, BUF_SIZE, GFP_KERNEL);
>>> +    if (!interrupt)
>>> +        return -ENOMEM;
>>> +
>>> +    snprintf(interrupt, BUF_SIZE, "q6v5_wcss_userpd%d_%s", 
>>> wcss->pd_asid, tmp);
>>> +
>>> +    ret = devm_request_threaded_irq(&pdev->dev, irq,
>>> +                    NULL, handler,
>>> +                    IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>>> +                    interrupt, q6);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "failed to acquire %s irq\n", interrupt);
>>> +        return ret;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +static int q6_get_outbound_irq(struct qcom_q6v5 *q6,
>>> +                   struct platform_device *pdev,
>>> +                   const char *int_name)
>>> +{
>>> +    struct qcom_smem_state *tmp_state;
>>> +    unsigned  bit;
>>> +
>>> +    tmp_state = qcom_smem_state_get(&pdev->dev, int_name, &bit);
>>> +    if (IS_ERR(tmp_state)) {
>>> +        dev_err(&pdev->dev, "failed to acquire %s state\n", int_name);
>>> +        return PTR_ERR(tmp_state);
>>> +    }
>>> +
>>> +    if (!strcmp(int_name, "stop")) {
>>> +        q6->state = tmp_state;
>>> +        q6->stop_bit = bit;
>>> +    } else if (!strcmp(int_name, "spawn")) {
>>> +        q6->spawn_state = tmp_state;
>>> +        q6->spawn_bit = bit;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int init_irq(struct qcom_q6v5 *q6,
>>> +            struct platform_device *pdev, struct rproc *rproc,
>>> +            int crash_reason, const char *load_state,
>>> +            void (*handover)(struct qcom_q6v5 *q6))
>>> +{
>>> +    int ret;
>>> +
>>> +    q6->rproc = rproc;
>>> +    q6->dev = &pdev->dev;
>>> +    q6->crash_reason = crash_reason;
>>> +    q6->handover = handover;
>>> +
>>> +    init_completion(&q6->start_done);
>>> +    init_completion(&q6->stop_done);
>>> +    init_completion(&q6->spawn_done);
>>> +
>>> +    ret = q6_get_inbound_irq(q6, pdev, "fatal",
>>> +                 q6v5_fatal_interrupt);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = q6_get_inbound_irq(q6, pdev, "ready",
>>> +                 q6v5_ready_interrupt);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = q6_get_inbound_irq(q6, pdev, "stop-ack",
>>> +                 q6v5_stop_interrupt);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = q6_get_inbound_irq(q6, pdev, "spawn-ack",
>>> +                 q6v5_spawn_interrupt);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = q6_get_outbound_irq(q6, pdev, "stop");
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = q6_get_outbound_irq(q6, pdev, "spawn");
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int q6_wcss_probe(struct platform_device *pdev)
>>> +{
>>> +    const struct wcss_data *desc;
>>> +    struct q6_wcss *wcss;
>>> +    struct rproc *rproc;
>>> +    int ret;
>>> +    char *subdev_name;
>>> +    const char **firmware;
>>> +
>>> +    desc = of_device_get_match_data(&pdev->dev);
>>> +    if (!desc)
>>> +        return -EINVAL;
>>> +
>>> +    firmware = devm_kcalloc(&pdev->dev, MAX_FIRMWARE,
>>> +                sizeof(*firmware), GFP_KERNEL);
>>> +    if (!firmware)
>>> +        return -ENOMEM;
>>> +
>>> +    ret = of_property_read_string_array(pdev->dev.of_node, 
>>> "firmware-name",
>>> +                        firmware, MAX_FIRMWARE);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops,
>>> +                firmware[0], sizeof(*wcss));
>>> +    if (!rproc) {
>>> +        dev_err(&pdev->dev, "failed to allocate rproc\n");
>>
>> ENOMEM do not print errors. Why do you have to print something here?
>>

	Yeah, I will remove print.

>>> +        return -ENOMEM;
>>> +    }
>>> +    wcss = rproc->priv;
>>> +    wcss->dev = &pdev->dev;
>>> +    wcss->desc = desc;
>>> +    wcss->firmware = firmware;
>>> +
>>> +    ret = q6_alloc_memory_region(wcss);
>>> +    if (ret)
>>> +        goto free_rproc;
>>> +
>>> +    wcss->pd_asid = qcom_get_pd_asid(wcss->dev->of_node);
>>> +    if (wcss->pd_asid < 0)
>>> +        goto free_rproc;
>>> +
>>> +    if (desc->init_irq) {
>>> +        ret = desc->init_irq(&wcss->q6, pdev, rproc,
>>> +                     desc->crash_reason_smem, NULL, NULL);
>>> +        if (ret)
>>> +            goto free_rproc;
>>> +    }
>>> +
>>> +    if (desc->glink_subdev_required)
>>> +        qcom_add_glink_subdev(rproc, &wcss->glink_subdev, 
>>> desc->ssr_name);
>>> +
>>> +    subdev_name = (char *)(desc->ssr_name ? desc->ssr_name : 
>>> pdev->name);
>>> +    qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, subdev_name);
>>> +
>>> +    rproc->auto_boot = false;
>>> +    ret = rproc_add(rproc);
>>> +    if (ret)
>>> +        goto free_rproc;
>>> +
>>> +    platform_set_drvdata(pdev, rproc);
>>> +
>>> +    ret = of_platform_populate(wcss->dev->of_node, NULL,
>>> +                   NULL, wcss->dev);
>>
>> It is the same probe used for the children, right? So whom do they 
>> populate?
>>

	I will add match table in 2nd param, so that childs, whose
	compatible matches with match table will be populated.

	Is this fine ?
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "failed to populate wcss pd nodes\n");
>>
>> dev_err_probe
>>
	 Sure, i will use dev_err_probe

>>> +        goto free_rproc;
>>> +    }
>>> +    return 0;
>>> +
>>> +free_rproc:
>>> +    rproc_free(rproc);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int q6_wcss_remove(struct platform_device *pdev)
>>> +{
>>> +    struct rproc *rproc = platform_get_drvdata(pdev);
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +
>>> +    qcom_q6v5_deinit(&wcss->q6);
>>> +
>>> +    rproc_del(rproc);
>>> +    rproc_free(rproc);
>>> +
>>> +    return 0;
>>> +}
>>
>>
>>

Thanks & Regards,
Manikanta.
Krzysztof Kozlowski July 1, 2023, 10:51 a.m. UTC | #8
On 30/06/2023 09:12, Manikanta Mylavarapu wrote:
> 
> 
> On 6/24/2023 12:49 PM, Krzysztof Kozlowski wrote:
>> On 21/06/2023 13:39, Manikanta Mylavarapu wrote:
>>>>> on number of wcss radios connected on that board and only one instance
>>>>> of 'qcom,ipq5018-q6-mpd'.
>>>>>
>>>>
>>>> I don't understand why the user protection domains need a specific
>>>> compatible. Why do they need compatible at all?
>>>>
>>>> Not mentioning that amount of your domains on Q6 is actually fixed per
>>>> SoC and probably should not be in DT at all.
>>>>
>>>     root domain is fixed per soc (One Q6 DSP, one per soc)
>>>     user domain(s) are variable (based on number of wcss radios attached)
>>>
>>>     The sequence to initialize, bring up, tear down the Q6 and wcss radios
>>>     are completely different. So in order to differentiate them, we will
>>>     need different compatibles. So this is a new rproc driver/architecture
>>>     which has a parent/child topology (Q6 DSP -> Master/parent controls
>>>     the WCSS (child)).
>>
>> I understand you need different properties, but I don't see yet the
>> benefit of creating here artificial compatibles. Look at your ipq9574
>> DTSI change - it does not use even ipq9574 compatibles for 66% of its
>> children.
>>
>> Maybe you should have there just property describing type of device or
>> bringup?
>>
> 
> 	Yeah i got your point. Indeed the requirement here is to
> 	have device specific compatibles, so will have just two
> 	compatible one for Q6-MPD and another for WCSS-MPD device's
> 
> 
> 	"qcom,q6-mpd" --> For Q6-MPD device
> 	"qcom,wcss-mpd" --> For WCSS-MPD device
> 
> 	Is this approach fine ?

Can you fix your reply style, so it is like on every mailing list? Some
weird indentation does no help to read it.

I was proposing to drop compatibles entirely. Making compatibles generic
is not solving any of my concerns.

I don't understand what do you want to achieve here and very limited
description of the hardware in the binding does not help.

> 
>> Given SoC cannot come with different amount of children (PD) and
>> different amount of radios. You even fix the firmware name, so
>> boards/customers cannot use anything else. It's fixed and the only
>> variable element here is disabling some of the blocks per board, if they
>> do not have physical interface (e.g. radio).
>>
>> You even hard-code the number of PD by using "pd-[123]", without unit
>> address, so you do not expect it will grow.
>>
>> Unless you want to say that these are devices? But your binding does not
>> really suggest it...
>>
>>
>> 	Yes, as i mentioned above the requirement is to have device

What requirement? You did not describe anything. Binding describes
hardware, not your requirements.

Binding said nothing about devices.

> 	specific bindings. We will remove "PD-X" from soc dtsi and
> 	add it in board dts file.

Why? How is it related to the bindings? What does it solve? Instead of
doing some changes you should explain why.

> 
> 	So soc dts would have "Q6-MPD" master node & board dts have
> 	"WCSS-MPD" child nodes based on number of radio's connected
> 	on board.
> 
> 	Is this fine ?
> 

Why?

Best regards,
Krzysztof