diff mbox series

[02/19] remoteproc: qcom: pas: Add QCS8300 remoteproc support

Message ID 20240904-qcs8300_initial_dtsi-v1-2-d0ea9afdc007@quicinc.com (mailing list archive)
State New
Headers show
Series Add initial support for QCS8300 | expand

Commit Message

Jingyi Wang Sept. 4, 2024, 8:33 a.m. UTC
Add support for PIL loading on ADSP, CDSP and GPDSP on QCS8300
platform.

Co-developed-by: Xin Liu <quic_liuxin@quicinc.com>
Signed-off-by: Xin Liu <quic_liuxin@quicinc.com>
Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
---
 drivers/remoteproc/qcom_q6v5_pas.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Krzysztof Kozlowski Sept. 4, 2024, 9:36 a.m. UTC | #1
On 04/09/2024 10:33, Jingyi Wang wrote:
> Add support for PIL loading on ADSP, CDSP and GPDSP on QCS8300
> platform.
> 
> Co-developed-by: Xin Liu <quic_liuxin@quicinc.com>
> Signed-off-by: Xin Liu <quic_liuxin@quicinc.com>
> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
> ---
>  drivers/remoteproc/qcom_q6v5_pas.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index ef82835e98a4..f92ccd4921b7 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -1416,6 +1416,9 @@ static const struct of_device_id adsp_of_match[] = {
>  	{ .compatible = "qcom,qcs404-adsp-pas", .data = &adsp_resource_init },
>  	{ .compatible = "qcom,qcs404-cdsp-pas", .data = &cdsp_resource_init },
>  	{ .compatible = "qcom,qcs404-wcss-pas", .data = &wcss_resource_init },
> +	{ .compatible = "qcom,qcs8300-adsp-pas", .data = &sa8775p_adsp_resource},
> +	{ .compatible = "qcom,qcs8300-cdsp-pas", .data = &sa8775p_cdsp0_resource},
> +	{ .compatible = "qcom,qcs8300-gpdsp-pas", .data = &sa8775p_gpdsp0_resource},

What's the point of this? You have entire commit msg to explain such
weird duplication. Otherwise sorry, don't duplicate unnecessarily.
Devices are compatible, aren't they?

Best regards,
Krzysztof
Jingyi Wang Sept. 5, 2024, 4:30 a.m. UTC | #2
On 9/4/2024 5:36 PM, Krzysztof Kozlowski wrote:
> On 04/09/2024 10:33, Jingyi Wang wrote:
>> Add support for PIL loading on ADSP, CDSP and GPDSP on QCS8300
>> platform.
>>
>> Co-developed-by: Xin Liu <quic_liuxin@quicinc.com>
>> Signed-off-by: Xin Liu <quic_liuxin@quicinc.com>
>> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
>> ---
>>  drivers/remoteproc/qcom_q6v5_pas.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
>> index ef82835e98a4..f92ccd4921b7 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pas.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
>> @@ -1416,6 +1416,9 @@ static const struct of_device_id adsp_of_match[] = {
>>  	{ .compatible = "qcom,qcs404-adsp-pas", .data = &adsp_resource_init },
>>  	{ .compatible = "qcom,qcs404-cdsp-pas", .data = &cdsp_resource_init },
>>  	{ .compatible = "qcom,qcs404-wcss-pas", .data = &wcss_resource_init },
>> +	{ .compatible = "qcom,qcs8300-adsp-pas", .data = &sa8775p_adsp_resource},
>> +	{ .compatible = "qcom,qcs8300-cdsp-pas", .data = &sa8775p_cdsp0_resource},
>> +	{ .compatible = "qcom,qcs8300-gpdsp-pas", .data = &sa8775p_gpdsp0_resource},
> 
> What's the point of this? You have entire commit msg to explain such
> weird duplication. Otherwise sorry, don't duplicate unnecessarily.
> Devices are compatible, aren't they?
> 
> Best regards,
> Krzysztof
> 
> 
I will drop this, could you please help us to understand what is the correct way to
deal such situation, do we need to update the yaml and add qcs8300 bindings or just
reference to sa8775p bindings in the device tree?

Thanks,
Jingyi
Krzysztof Kozlowski Sept. 5, 2024, 6:24 a.m. UTC | #3
On 05/09/2024 06:30, Jingyi Wang wrote:
>>> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
>>> index ef82835e98a4..f92ccd4921b7 100644
>>> --- a/drivers/remoteproc/qcom_q6v5_pas.c
>>> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
>>> @@ -1416,6 +1416,9 @@ static const struct of_device_id adsp_of_match[] = {
>>>  	{ .compatible = "qcom,qcs404-adsp-pas", .data = &adsp_resource_init },
>>>  	{ .compatible = "qcom,qcs404-cdsp-pas", .data = &cdsp_resource_init },
>>>  	{ .compatible = "qcom,qcs404-wcss-pas", .data = &wcss_resource_init },
>>> +	{ .compatible = "qcom,qcs8300-adsp-pas", .data = &sa8775p_adsp_resource},
>>> +	{ .compatible = "qcom,qcs8300-cdsp-pas", .data = &sa8775p_cdsp0_resource},
>>> +	{ .compatible = "qcom,qcs8300-gpdsp-pas", .data = &sa8775p_gpdsp0_resource},
>>
>> What's the point of this? You have entire commit msg to explain such
>> weird duplication. Otherwise sorry, don't duplicate unnecessarily.
>> Devices are compatible, aren't they?
>>
>> Best regards,
>> Krzysztof
>>
>>
> I will drop this, could you please help us to understand what is the correct way to
> deal such situation, do we need to update the yaml and add qcs8300 bindings or just
> reference to sa8775p bindings in the device tree?

Above diff hunk suggests that devices are compatible, so should be made
compatible in the bindings (use fallback). There are plenty examples of
this for all Qualcomm devices.

Best regards,
Krzysztof
Jingyi Wang Sept. 6, 2024, 5:33 a.m. UTC | #4
On 9/5/2024 2:24 PM, Krzysztof Kozlowski wrote:
> On 05/09/2024 06:30, Jingyi Wang wrote:
>>>> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
>>>> index ef82835e98a4..f92ccd4921b7 100644
>>>> --- a/drivers/remoteproc/qcom_q6v5_pas.c
>>>> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
>>>> @@ -1416,6 +1416,9 @@ static const struct of_device_id adsp_of_match[] = {
>>>>  	{ .compatible = "qcom,qcs404-adsp-pas", .data = &adsp_resource_init },
>>>>  	{ .compatible = "qcom,qcs404-cdsp-pas", .data = &cdsp_resource_init },
>>>>  	{ .compatible = "qcom,qcs404-wcss-pas", .data = &wcss_resource_init },
>>>> +	{ .compatible = "qcom,qcs8300-adsp-pas", .data = &sa8775p_adsp_resource},
>>>> +	{ .compatible = "qcom,qcs8300-cdsp-pas", .data = &sa8775p_cdsp0_resource},
>>>> +	{ .compatible = "qcom,qcs8300-gpdsp-pas", .data = &sa8775p_gpdsp0_resource},
>>>
>>> What's the point of this? You have entire commit msg to explain such
>>> weird duplication. Otherwise sorry, don't duplicate unnecessarily.
>>> Devices are compatible, aren't they?
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>>
>> I will drop this, could you please help us to understand what is the correct way to
>> deal such situation, do we need to update the yaml and add qcs8300 bindings or just
>> reference to sa8775p bindings in the device tree?
> 
> Above diff hunk suggests that devices are compatible, so should be made
> compatible in the bindings (use fallback). There are plenty examples of
> this for all Qualcomm devices.
> 
> Best regards,
> Krzysztof
> 

The usage of binding seems inconsistent across different Qualcomm drivers. Could you please
confirm that when you mentioned "use fallback", do you mean binding like this?

      - items:
          - enum:
              - qcom,sm8550-sndcard
              - qcom,sm8650-sndcard
          - const: qcom,sm8450-sndcard

https://www.kernel.org/doc/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml

Thanks,
Jingyi
diff mbox series

Patch

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index ef82835e98a4..f92ccd4921b7 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -1416,6 +1416,9 @@  static const struct of_device_id adsp_of_match[] = {
 	{ .compatible = "qcom,qcs404-adsp-pas", .data = &adsp_resource_init },
 	{ .compatible = "qcom,qcs404-cdsp-pas", .data = &cdsp_resource_init },
 	{ .compatible = "qcom,qcs404-wcss-pas", .data = &wcss_resource_init },
+	{ .compatible = "qcom,qcs8300-adsp-pas", .data = &sa8775p_adsp_resource},
+	{ .compatible = "qcom,qcs8300-cdsp-pas", .data = &sa8775p_cdsp0_resource},
+	{ .compatible = "qcom,qcs8300-gpdsp-pas", .data = &sa8775p_gpdsp0_resource},
 	{ .compatible = "qcom,sa8775p-adsp-pas", .data = &sa8775p_adsp_resource},
 	{ .compatible = "qcom,sa8775p-cdsp0-pas", .data = &sa8775p_cdsp0_resource},
 	{ .compatible = "qcom,sa8775p-cdsp1-pas", .data = &sa8775p_cdsp1_resource},