Re: [PATCH V2 01/13] dt-bindings: remoteproc: qcom: Add support for multipd model

[Krzysztof Kozlowski]

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