diff mbox series

[v2,01/16] dt-bindings: dma: qcom,bam: Add bam pipe lock

Message ID 20240815085725.2740390-2-quic_mdalam@quicinc.com (mailing list archive)
State Not Applicable
Headers show
Series Add cmd descriptor support | expand

Commit Message

Md Sadre Alam Aug. 15, 2024, 8:57 a.m. UTC
BAM having pipe locking mechanism. The Lock and Un-Lock bit
should be set on CMD descriptor only. Upon encountering a
descriptor with Lock bit set, the BAM will lock all other
pipes not related to the current pipe group, and keep
handling the current pipe only until it sees the Un-Lock
set.

Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---

Change in [v2]

* Added initial support for dt-binding

Change in [v1]

* This patch was not included in [v1]

 Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Bjorn Andersson Aug. 16, 2024, 3:53 p.m. UTC | #1
On Thu, Aug 15, 2024 at 02:27:10PM GMT, Md Sadre Alam wrote:
> BAM having pipe locking mechanism. The Lock and Un-Lock bit
> should be set on CMD descriptor only. Upon encountering a
> descriptor with Lock bit set, the BAM will lock all other
> pipes not related to the current pipe group, and keep
> handling the current pipe only until it sees the Un-Lock
> set.

This describes the mechanism for mutual exclusion, but not really what
the patch does.

https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format
states that you have 75 characters for your commit message, use them.

> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> ---
> 
> Change in [v2]
> 
> * Added initial support for dt-binding
> 
> Change in [v1]
> 
> * This patch was not included in [v1]
> 
>  Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> index 3ad0d9b1fbc5..91cc2942aa62 100644
> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> @@ -77,6 +77,12 @@ properties:
>        Indicates that the bam is powered up by a remote processor but must be
>        initialized by the local processor.
>  
> +  qcom,bam_pipe_lock:

'_' is not a valid character in node names or properties.

> +    type: boolean
> +    description:
> +      Indicates that the bam pipe needs locking or not based on client driver
> +      sending the LOCK or UNLOK bit set on command descriptor.

Missing 'C'?

Regards,
Bjorn

> +
>    reg:
>      maxItems: 1
>  
> @@ -92,6 +98,8 @@ anyOf:
>        - qcom,powered-remotely
>    - required:
>        - qcom,controlled-remotely
> +  - required:
> +      - qcom,bam_pipe_lock
>    - required:
>        - clocks
>        - clock-names
> -- 
> 2.34.1
>
Krzysztof Kozlowski Aug. 17, 2024, 9:08 a.m. UTC | #2
On 15/08/2024 10:57, Md Sadre Alam wrote:
> BAM having pipe locking mechanism. The Lock and Un-Lock bit
> should be set on CMD descriptor only. Upon encountering a
> descriptor with Lock bit set, the BAM will lock all other
> pipes not related to the current pipe group, and keep
> handling the current pipe only until it sees the Un-Lock
> set.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> ---
> 
> Change in [v2]
> 
> * Added initial support for dt-binding
> 
> Change in [v1]
> 
> * This patch was not included in [v1]
> 
>  Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> index 3ad0d9b1fbc5..91cc2942aa62 100644
> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> @@ -77,6 +77,12 @@ properties:
>        Indicates that the bam is powered up by a remote processor but must be
>        initialized by the local processor.
>  
> +  qcom,bam_pipe_lock:

Please follow DTS coding style.

> +    type: boolean
> +    description:
> +      Indicates that the bam pipe needs locking or not based on client driver
> +      sending the LOCK or UNLOK bit set on command descriptor.

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

> +
>    reg:
>      maxItems: 1
>  
> @@ -92,6 +98,8 @@ anyOf:
>        - qcom,powered-remotely
>    - required:
>        - qcom,controlled-remotely
> +  - required:
> +      - qcom,bam_pipe_lock

Why is it here? What do you want to achieve?

>    - required:
>        - clocks
>        - clock-names

Best regards,
Krzysztof
Md Sadre Alam Aug. 21, 2024, 4:54 a.m. UTC | #3
On 8/16/2024 9:23 PM, Bjorn Andersson wrote:
> On Thu, Aug 15, 2024 at 02:27:10PM GMT, Md Sadre Alam wrote:
>> BAM having pipe locking mechanism. The Lock and Un-Lock bit
>> should be set on CMD descriptor only. Upon encountering a
>> descriptor with Lock bit set, the BAM will lock all other
>> pipes not related to the current pipe group, and keep
>> handling the current pipe only until it sees the Un-Lock
>> set.
> 
> This describes the mechanism for mutual exclusion, but not really what
> the patch does.
   Ok, will update in next patch.
> 
> https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format
> states that you have 75 characters for your commit message, use them.
   ok, will update in next patch.
> 
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
>>
>> Change in [v2]
>>
>> * Added initial support for dt-binding
>>
>> Change in [v1]
>>
>> * This patch was not included in [v1]
>>
>>   Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>> index 3ad0d9b1fbc5..91cc2942aa62 100644
>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>> @@ -77,6 +77,12 @@ properties:
>>         Indicates that the bam is powered up by a remote processor but must be
>>         initialized by the local processor.
>>   
>> +  qcom,bam_pipe_lock:
> 
> '_' is not a valid character in node names or properties.
    Ok, will update in next patch.
> 
>> +    type: boolean
>> +    description:
>> +      Indicates that the bam pipe needs locking or not based on client driver
>> +      sending the LOCK or UNLOK bit set on command descriptor.
> 
> Missing 'C'?
   Ok, will fix in next patch.
> 
> Regards,
> Bjorn
> 
>> +
>>     reg:
>>       maxItems: 1
>>   
>> @@ -92,6 +98,8 @@ anyOf:
>>         - qcom,powered-remotely
>>     - required:
>>         - qcom,controlled-remotely
>> +  - required:
>> +      - qcom,bam_pipe_lock
>>     - required:
>>         - clocks
>>         - clock-names
>> -- 
>> 2.34.1
>>
Md Sadre Alam Aug. 21, 2024, 4:34 p.m. UTC | #4
On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote:
> On 15/08/2024 10:57, Md Sadre Alam wrote:
>> BAM having pipe locking mechanism. The Lock and Un-Lock bit
>> should be set on CMD descriptor only. Upon encountering a
>> descriptor with Lock bit set, the BAM will lock all other
>> pipes not related to the current pipe group, and keep
>> handling the current pipe only until it sees the Un-Lock
>> set.
> 
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
   Ok , will update in next patch.
> 
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
>>
>> Change in [v2]
>>
>> * Added initial support for dt-binding
>>
>> Change in [v1]
>>
>> * This patch was not included in [v1]
>>
>>   Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>> index 3ad0d9b1fbc5..91cc2942aa62 100644
>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>> @@ -77,6 +77,12 @@ properties:
>>         Indicates that the bam is powered up by a remote processor but must be
>>         initialized by the local processor.
>>   
>> +  qcom,bam_pipe_lock:
> 
> Please follow DTS coding style.
   Ok
> 
>> +    type: boolean
>> +    description:
>> +      Indicates that the bam pipe needs locking or not based on client driver
>> +      sending the LOCK or UNLOK bit set on command descriptor.
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
   Ok, will update in next patch.
> 
>> +
>>     reg:
>>       maxItems: 1
>>   
>> @@ -92,6 +98,8 @@ anyOf:
>>         - qcom,powered-remotely
>>     - required:
>>         - qcom,controlled-remotely
>> +  - required:
>> +      - qcom,bam_pipe_lock
> 
> Why is it here? What do you want to achieve?
   This property added to achieve locking/unlocking
   of BAM pipe groups for mutual exclusion of resources
   that can be used across multiple EE's
> 
>>     - required:
>>         - clocks
>>         - clock-names
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Aug. 22, 2024, 6:27 a.m. UTC | #5
On 21/08/2024 18:34, Md Sadre Alam wrote:
> 
> 
> On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote:
>> On 15/08/2024 10:57, Md Sadre Alam wrote:
>>> BAM having pipe locking mechanism. The Lock and Un-Lock bit
>>> should be set on CMD descriptor only. Upon encountering a
>>> descriptor with Lock bit set, the BAM will lock all other
>>> pipes not related to the current pipe group, and keep
>>> handling the current pipe only until it sees the Un-Lock
>>> set.
>>
>> Please wrap commit message according to Linux coding style / submission
>> process (neither too early nor over the limit):
>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>    Ok , will update in next patch.
>>
>>>
>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>>> ---
>>>
>>> Change in [v2]
>>>
>>> * Added initial support for dt-binding
>>>
>>> Change in [v1]
>>>
>>> * This patch was not included in [v1]
>>>
>>>   Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>> index 3ad0d9b1fbc5..91cc2942aa62 100644
>>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>> @@ -77,6 +77,12 @@ properties:
>>>         Indicates that the bam is powered up by a remote processor but must be
>>>         initialized by the local processor.
>>>   
>>> +  qcom,bam_pipe_lock:
>>
>> Please follow DTS coding style.
>    Ok
>>
>>> +    type: boolean
>>> +    description:
>>> +      Indicates that the bam pipe needs locking or not based on client driver
>>> +      sending the LOCK or UNLOK bit set on command descriptor.
>>
>> You described the desired Linux feature or behavior, not the actual
>> hardware. The bindings are about the latter, so instead you need to
>> rephrase the property and its description to match actual hardware
>> capabilities/features/configuration etc.
>    Ok, will update in next patch.
>>
>>> +
>>>     reg:
>>>       maxItems: 1
>>>   
>>> @@ -92,6 +98,8 @@ anyOf:
>>>         - qcom,powered-remotely
>>>     - required:
>>>         - qcom,controlled-remotely
>>> +  - required:
>>> +      - qcom,bam_pipe_lock
>>
>> Why is it here? What do you want to achieve?
>    This property added to achieve locking/unlocking
>    of BAM pipe groups for mutual exclusion of resources
>    that can be used across multiple EE's

This explains me nothing. I am questioning the anyOf block. Why this is
the fourth method of controlling BAM? Anyway, if it is, then explain
this in commit msg.

Best regards,
Krzysztof
Md Sadre Alam Aug. 22, 2024, 11:45 a.m. UTC | #6
On 8/22/2024 11:57 AM, Krzysztof Kozlowski wrote:
> On 21/08/2024 18:34, Md Sadre Alam wrote:
>>
>>
>> On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote:
>>> On 15/08/2024 10:57, Md Sadre Alam wrote:
>>>> BAM having pipe locking mechanism. The Lock and Un-Lock bit
>>>> should be set on CMD descriptor only. Upon encountering a
>>>> descriptor with Lock bit set, the BAM will lock all other
>>>> pipes not related to the current pipe group, and keep
>>>> handling the current pipe only until it sees the Un-Lock
>>>> set.
>>>
>>> Please wrap commit message according to Linux coding style / submission
>>> process (neither too early nor over the limit):
>>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>     Ok , will update in next patch.
>>>
>>>>
>>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>>>> ---
>>>>
>>>> Change in [v2]
>>>>
>>>> * Added initial support for dt-binding
>>>>
>>>> Change in [v1]
>>>>
>>>> * This patch was not included in [v1]
>>>>
>>>>    Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>> index 3ad0d9b1fbc5..91cc2942aa62 100644
>>>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>> @@ -77,6 +77,12 @@ properties:
>>>>          Indicates that the bam is powered up by a remote processor but must be
>>>>          initialized by the local processor.
>>>>    
>>>> +  qcom,bam_pipe_lock:
>>>
>>> Please follow DTS coding style.
>>     Ok
>>>
>>>> +    type: boolean
>>>> +    description:
>>>> +      Indicates that the bam pipe needs locking or not based on client driver
>>>> +      sending the LOCK or UNLOK bit set on command descriptor.
>>>
>>> You described the desired Linux feature or behavior, not the actual
>>> hardware. The bindings are about the latter, so instead you need to
>>> rephrase the property and its description to match actual hardware
>>> capabilities/features/configuration etc.
>>     Ok, will update in next patch.
>>>
>>>> +
>>>>      reg:
>>>>        maxItems: 1
>>>>    
>>>> @@ -92,6 +98,8 @@ anyOf:
>>>>          - qcom,powered-remotely
>>>>      - required:
>>>>          - qcom,controlled-remotely
>>>> +  - required:
>>>> +      - qcom,bam_pipe_lock
>>>
>>> Why is it here? What do you want to achieve?
>>     This property added to achieve locking/unlocking
>>     of BAM pipe groups for mutual exclusion of resources
>>     that can be used across multiple EE's
> 
> This explains me nothing. I am questioning the anyOf block. Why this is
> the fourth method of controlling BAM? Anyway, if it is, then explain
> this in commit msg.
   This is the BAM property for locking/unlocking the BAM pipes.That's
   why I kept in anyOf block.
   Will explain in commit message in next patch.
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Aug. 23, 2024, 9:07 a.m. UTC | #7
On 22/08/2024 13:45, Md Sadre Alam wrote:
> 
> 
> On 8/22/2024 11:57 AM, Krzysztof Kozlowski wrote:
>> On 21/08/2024 18:34, Md Sadre Alam wrote:
>>>
>>>
>>> On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote:
>>>> On 15/08/2024 10:57, Md Sadre Alam wrote:
>>>>> BAM having pipe locking mechanism. The Lock and Un-Lock bit
>>>>> should be set on CMD descriptor only. Upon encountering a
>>>>> descriptor with Lock bit set, the BAM will lock all other
>>>>> pipes not related to the current pipe group, and keep
>>>>> handling the current pipe only until it sees the Un-Lock
>>>>> set.
>>>>
>>>> Please wrap commit message according to Linux coding style / submission
>>>> process (neither too early nor over the limit):
>>>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>>     Ok , will update in next patch.
>>>>
>>>>>
>>>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>>>>> ---
>>>>>
>>>>> Change in [v2]
>>>>>
>>>>> * Added initial support for dt-binding
>>>>>
>>>>> Change in [v1]
>>>>>
>>>>> * This patch was not included in [v1]
>>>>>
>>>>>    Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
>>>>>    1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>>> index 3ad0d9b1fbc5..91cc2942aa62 100644
>>>>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>>> @@ -77,6 +77,12 @@ properties:
>>>>>          Indicates that the bam is powered up by a remote processor but must be
>>>>>          initialized by the local processor.
>>>>>    
>>>>> +  qcom,bam_pipe_lock:
>>>>
>>>> Please follow DTS coding style.
>>>     Ok
>>>>
>>>>> +    type: boolean
>>>>> +    description:
>>>>> +      Indicates that the bam pipe needs locking or not based on client driver
>>>>> +      sending the LOCK or UNLOK bit set on command descriptor.
>>>>
>>>> You described the desired Linux feature or behavior, not the actual
>>>> hardware. The bindings are about the latter, so instead you need to
>>>> rephrase the property and its description to match actual hardware
>>>> capabilities/features/configuration etc.
>>>     Ok, will update in next patch.
>>>>
>>>>> +
>>>>>      reg:
>>>>>        maxItems: 1
>>>>>    
>>>>> @@ -92,6 +98,8 @@ anyOf:
>>>>>          - qcom,powered-remotely
>>>>>      - required:
>>>>>          - qcom,controlled-remotely
>>>>> +  - required:
>>>>> +      - qcom,bam_pipe_lock
>>>>
>>>> Why is it here? What do you want to achieve?
>>>     This property added to achieve locking/unlocking
>>>     of BAM pipe groups for mutual exclusion of resources
>>>     that can be used across multiple EE's
>>
>> This explains me nothing. I am questioning the anyOf block. Why this is
>> the fourth method of controlling BAM? Anyway, if it is, then explain
>> this in commit msg.
>    This is the BAM property for locking/unlocking the BAM pipes.That's
>    why I kept in anyOf block.

You keep repeating the same. It's like poking me with the same comment
till I agree. I am done with this.

NAK. Provide proper rationale.

Best regards,
Krzysztof
Manivannan Sadhasivam Aug. 23, 2024, 3:39 p.m. UTC | #8
On Thu, Aug 15, 2024 at 02:27:10PM +0530, Md Sadre Alam wrote:
> BAM having pipe locking mechanism. The Lock and Un-Lock bit
> should be set on CMD descriptor only. Upon encountering a
> descriptor with Lock bit set, the BAM will lock all other
> pipes not related to the current pipe group, and keep
> handling the current pipe only until it sees the Un-Lock
> set.
> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> ---
> 
> Change in [v2]
> 
> * Added initial support for dt-binding
> 
> Change in [v1]
> 
> * This patch was not included in [v1]
> 
>  Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> index 3ad0d9b1fbc5..91cc2942aa62 100644
> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> @@ -77,6 +77,12 @@ properties:
>        Indicates that the bam is powered up by a remote processor but must be
>        initialized by the local processor.
>  
> +  qcom,bam_pipe_lock:
> +    type: boolean
> +    description:
> +      Indicates that the bam pipe needs locking or not based on client driver
> +      sending the LOCK or UNLOK bit set on command descriptor.
> +

This looks like a pure driver implementation and doesn't belong to the DT at
all. Why can't you add a logic in the driver to use the lock based on some
detection mechanism?

- Mani

>    reg:
>      maxItems: 1
>  
> @@ -92,6 +98,8 @@ anyOf:
>        - qcom,powered-remotely
>    - required:
>        - qcom,controlled-remotely
> +  - required:
> +      - qcom,bam_pipe_lock
>    - required:
>        - clocks
>        - clock-names
> -- 
> 2.34.1
> 
>
Md Sadre Alam Aug. 24, 2024, 7:04 a.m. UTC | #9
On 8/23/2024 9:09 PM, Manivannan Sadhasivam wrote:
> On Thu, Aug 15, 2024 at 02:27:10PM +0530, Md Sadre Alam wrote:
>> BAM having pipe locking mechanism. The Lock and Un-Lock bit
>> should be set on CMD descriptor only. Upon encountering a
>> descriptor with Lock bit set, the BAM will lock all other
>> pipes not related to the current pipe group, and keep
>> handling the current pipe only until it sees the Un-Lock
>> set.
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
>>
>> Change in [v2]
>>
>> * Added initial support for dt-binding
>>
>> Change in [v1]
>>
>> * This patch was not included in [v1]
>>
>>   Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>> index 3ad0d9b1fbc5..91cc2942aa62 100644
>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>> @@ -77,6 +77,12 @@ properties:
>>         Indicates that the bam is powered up by a remote processor but must be
>>         initialized by the local processor.
>>   
>> +  qcom,bam_pipe_lock:
>> +    type: boolean
>> +    description:
>> +      Indicates that the bam pipe needs locking or not based on client driver
>> +      sending the LOCK or UNLOK bit set on command descriptor.
>> +
> 
> This looks like a pure driver implementation and doesn't belong to the DT at
> all. Why can't you add a logic in the driver to use the lock based on some
> detection mechanism?
   Sure , will use BAM_SW_VERSION register for detection mechanism, since this
   support only for bam version above 1.4.0.
> 
> - Mani
> 
>>     reg:
>>       maxItems: 1
>>   
>> @@ -92,6 +98,8 @@ anyOf:
>>         - qcom,powered-remotely
>>     - required:
>>         - qcom,controlled-remotely
>> +  - required:
>> +      - qcom,bam_pipe_lock
>>     - required:
>>         - clocks
>>         - clock-names
>> -- 
>> 2.34.1
>>
>>
>
Md Sadre Alam Aug. 24, 2024, 7:07 a.m. UTC | #10
On 8/23/2024 2:37 PM, Krzysztof Kozlowski wrote:
> On 22/08/2024 13:45, Md Sadre Alam wrote:
>>
>>
>> On 8/22/2024 11:57 AM, Krzysztof Kozlowski wrote:
>>> On 21/08/2024 18:34, Md Sadre Alam wrote:
>>>>
>>>>
>>>> On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote:
>>>>> On 15/08/2024 10:57, Md Sadre Alam wrote:
>>>>>> BAM having pipe locking mechanism. The Lock and Un-Lock bit
>>>>>> should be set on CMD descriptor only. Upon encountering a
>>>>>> descriptor with Lock bit set, the BAM will lock all other
>>>>>> pipes not related to the current pipe group, and keep
>>>>>> handling the current pipe only until it sees the Un-Lock
>>>>>> set.
>>>>>
>>>>> Please wrap commit message according to Linux coding style / submission
>>>>> process (neither too early nor over the limit):
>>>>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>>>      Ok , will update in next patch.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>>>>>> ---
>>>>>>
>>>>>> Change in [v2]
>>>>>>
>>>>>> * Added initial support for dt-binding
>>>>>>
>>>>>> Change in [v1]
>>>>>>
>>>>>> * This patch was not included in [v1]
>>>>>>
>>>>>>     Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
>>>>>>     1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>>>> index 3ad0d9b1fbc5..91cc2942aa62 100644
>>>>>> --- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>>>>>> @@ -77,6 +77,12 @@ properties:
>>>>>>           Indicates that the bam is powered up by a remote processor but must be
>>>>>>           initialized by the local processor.
>>>>>>     
>>>>>> +  qcom,bam_pipe_lock:
>>>>>
>>>>> Please follow DTS coding style.
>>>>      Ok
>>>>>
>>>>>> +    type: boolean
>>>>>> +    description:
>>>>>> +      Indicates that the bam pipe needs locking or not based on client driver
>>>>>> +      sending the LOCK or UNLOK bit set on command descriptor.
>>>>>
>>>>> You described the desired Linux feature or behavior, not the actual
>>>>> hardware. The bindings are about the latter, so instead you need to
>>>>> rephrase the property and its description to match actual hardware
>>>>> capabilities/features/configuration etc.
>>>>      Ok, will update in next patch.
>>>>>
>>>>>> +
>>>>>>       reg:
>>>>>>         maxItems: 1
>>>>>>     
>>>>>> @@ -92,6 +98,8 @@ anyOf:
>>>>>>           - qcom,powered-remotely
>>>>>>       - required:
>>>>>>           - qcom,controlled-remotely
>>>>>> +  - required:
>>>>>> +      - qcom,bam_pipe_lock
>>>>>
>>>>> Why is it here? What do you want to achieve?
>>>>      This property added to achieve locking/unlocking
>>>>      of BAM pipe groups for mutual exclusion of resources
>>>>      that can be used across multiple EE's
>>>
>>> This explains me nothing. I am questioning the anyOf block. Why this is
>>> the fourth method of controlling BAM? Anyway, if it is, then explain
>>> this in commit msg.
>>     This is the BAM property for locking/unlocking the BAM pipes.That's
>>     why I kept in anyOf block.
> 
> You keep repeating the same. It's like poking me with the same comment
> till I agree. I am done with this.
> 
> NAK. Provide proper rationale.
   Sorry, I misunderstood your review comment. Now as Mani suggested will
   keep this implementation in driver itself. Will drop the binding patch
   in next revision.
> 
> Best regards,
> Krzysztof
> 
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
index 3ad0d9b1fbc5..91cc2942aa62 100644
--- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
+++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
@@ -77,6 +77,12 @@  properties:
       Indicates that the bam is powered up by a remote processor but must be
       initialized by the local processor.
 
+  qcom,bam_pipe_lock:
+    type: boolean
+    description:
+      Indicates that the bam pipe needs locking or not based on client driver
+      sending the LOCK or UNLOK bit set on command descriptor.
+
   reg:
     maxItems: 1
 
@@ -92,6 +98,8 @@  anyOf:
       - qcom,powered-remotely
   - required:
       - qcom,controlled-remotely
+  - required:
+      - qcom,bam_pipe_lock
   - required:
       - clocks
       - clock-names