[v2,6/6] dt-bindings: dma: Convert Qualcomm BAM DMA binding to json format

Convert Qualcomm BAM DMA controller binding to DT schema format using
json schema.

Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com>
---
 .../devicetree/bindings/dma/qcom,bam-dma.yaml | 94 +++++++++++++++++++
 .../devicetree/bindings/dma/qcom_bam_dma.txt  | 52 ----------
 2 files changed, 94 insertions(+), 52 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
 delete mode 100644 Documentation/devicetree/bindings/dma/qcom_bam_dma.txt

On 10/04/2022 19:50, Kuldeep Singh wrote:
> Convert Qualcomm BAM DMA controller binding to DT schema format using
> json schema.

Thank you for your patch. There is something to discuss/improve.

(...)

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  iommus:
> +    minItems: 1
> +    maxItems: 4

This is something new and it seems only one SoC defines it (not even one
BAM version). I wonder whether this is actually correct or this
particular version of BAM is slightly different. Maybe someone could
clarify it, but if no - looks ok.

> +
> +  num-channels:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Indicates supported number of DMA channels in a remotely controlled bam.
> +
> +  qcom,controlled-remotely:
> +    $ref: /schemas/types.yaml#/definitions/flag

type: boolean

> +    description:
> +      Indicates that the bam is controlled by remote proccessor i.e. execution
> +      environment.
> +
> +  qcom,ee:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Indicates the active Execution Environment identifier (0-7) used in the
> +      secure world.

maximum: 7

> +
> +  qcom,num-ees:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Indicates supported number of Execution Environments in a remotely
> +      controlled bam.
> +
> +  qcom,powered-remotely:
> +    $ref: /schemas/types.yaml#/definitions/flag

type: boolean

> +    description:
> +      Indicates that the bam is powered up by a remote processor but must be
> +      initialized by the local processor.
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - "#dma-cells"
> +  - interrupts
> +  - reg

clocks, clock-names, qcom-ee - these are required according to old bindings.


Best regards,
Krzysztof

> This is something new and it seems only one SoC defines it (not even one
> BAM version). I wonder whether this is actually correct or this
> particular version of BAM is slightly different. Maybe someone could
> clarify it, but if no - looks ok.

Yes, sdm845.dtsi uses 4 entries and rest 1.

> 
> > +
> > +  num-channels:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Indicates supported number of DMA channels in a remotely controlled bam.
> > +
> > +  qcom,controlled-remotely:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> 
> type: boolean

Boolean comes under flag in types.yaml

definitions:
  flag:
    oneOf:
      - type: boolean
        const: true
      - type: 'null'

I have seen other boolean properties(spi-cpol, spi-cpha and bunch of
others) using type flag. I think we should keep flag here.

> 
> > +    description:
> > +      Indicates that the bam is controlled by remote proccessor i.e. execution
> > +      environment.
> > +
> > +  qcom,ee:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Indicates the active Execution Environment identifier (0-7) used in the
> > +      secure world.
> 
> maximum: 7

ok.

> > +required:
> > +  - compatible
> > +  - "#dma-cells"
> > +  - interrupts
> > +  - reg
> 
> clocks, clock-names, qcom-ee - these are required according to old bindings.

I missed qcom,ee. Will add in v3.

For clocks and clock-names , there are two platforms(msm8996.dtsi,
sdm845.dtsi) where these properties are missing. And I don't want to add
some random values. Shall I skip them here? and let board owners add
them later.

On 11/04/2022 12:58, Kuldeep Singh wrote:
>> This is something new and it seems only one SoC defines it (not even one
>> BAM version). I wonder whether this is actually correct or this
>> particular version of BAM is slightly different. Maybe someone could
>> clarify it, but if no - looks ok.
> 
> Yes, sdm845.dtsi uses 4 entries and rest 1.

Yes, I know. This does not solve my wonder.

> 
>>
>>> +
>>> +  num-channels:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Indicates supported number of DMA channels in a remotely controlled bam.
>>> +
>>> +  qcom,controlled-remotely:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>
>> type: boolean
> 
> Boolean comes under flag in types.yaml
> 
> definitions:
>   flag:
>     oneOf:
>       - type: boolean
>         const: true
>       - type: 'null'
> 
> I have seen other boolean properties(spi-cpol, spi-cpha and bunch of
> others) using type flag. I think we should keep flag here.

type:boolean is just shorter and example-schema recommends it. If you
want to base on something (as a template, pattern) then the
example-schema is the source, the preferred one.

>>> +required:
>>> +  - compatible
>>> +  - "#dma-cells"
>>> +  - interrupts
>>> +  - reg
>>
>> clocks, clock-names, qcom-ee - these are required according to old bindings.
> 
> I missed qcom,ee. Will add in v3.
> 
> For clocks and clock-names , there are two platforms(msm8996.dtsi,
> sdm845.dtsi) where these properties are missing. And I don't want to add
> some random values. Shall I skip them here? and let board owners add
> them later.

These are required, so the SoC DTSI should be fixed. Not with random
clocks but something proper. :)

Best regards,
Krzysztof

On Mon, Apr 11, 2022 at 01:38:41PM +0200, Krzysztof Kozlowski wrote:
> On 11/04/2022 12:58, Kuldeep Singh wrote:
> >> This is something new and it seems only one SoC defines it (not even one
> >> BAM version). I wonder whether this is actually correct or this
> >> particular version of BAM is slightly different. Maybe someone could
> >> clarify it, but if no - looks ok.
> > 
> > Yes, sdm845.dtsi uses 4 entries and rest 1.
> 
> Yes, I know. This does not solve my wonder.
> 
> > 
> >>
> >>> +
> >>> +  num-channels:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    description:
> >>> +      Indicates supported number of DMA channels in a remotely controlled bam.
> >>> +
> >>> +  qcom,controlled-remotely:
> >>> +    $ref: /schemas/types.yaml#/definitions/flag
> >>
> >> type: boolean
> > 
> > Boolean comes under flag in types.yaml
> > 
> > definitions:
> >   flag:
> >     oneOf:
> >       - type: boolean
> >         const: true
> >       - type: 'null'
> > 
> > I have seen other boolean properties(spi-cpol, spi-cpha and bunch of
> > others) using type flag. I think we should keep flag here.
> 
> type:boolean is just shorter and example-schema recommends it. If you
> want to base on something (as a template, pattern) then the
> example-schema is the source, the preferred one.

I had seen other spec using flag and that's why kept same here.
Which example schema are you talking about?

> >>
> >> clocks, clock-names, qcom-ee - these are required according to old bindings.
> > 
> > I missed qcom,ee. Will add in v3.
> > 
> > For clocks and clock-names , there are two platforms(msm8996.dtsi,
> > sdm845.dtsi) where these properties are missing. And I don't want to add
> > some random values. Shall I skip them here? and let board owners add
> > them later.
> 
> These are required, so the SoC DTSI should be fixed. Not with random
> clocks but something proper. :)

Yes absolutely :-)
I have kept Srinivas in copy, who sent initial support for both the
dtsi. Probably he can confirm provided his email doesn't bounce.

Anyway Krzysztof, can you confirm the same as you have been actively
contributing to Qcom peripherals. I will add credit in follow-up
submission.

On 12/04/2022 08:19, Kuldeep Singh wrote:
> On Mon, Apr 11, 2022 at 01:38:41PM +0200, Krzysztof Kozlowski wrote:
>> On 11/04/2022 12:58, Kuldeep Singh wrote:
>>>> This is something new and it seems only one SoC defines it (not even one
>>>> BAM version). I wonder whether this is actually correct or this
>>>> particular version of BAM is slightly different. Maybe someone could
>>>> clarify it, but if no - looks ok.
>>>
>>> Yes, sdm845.dtsi uses 4 entries and rest 1.
>>
>> Yes, I know. This does not solve my wonder.
>>
>>>
>>>>
>>>>> +
>>>>> +  num-channels:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    description:
>>>>> +      Indicates supported number of DMA channels in a remotely controlled bam.
>>>>> +
>>>>> +  qcom,controlled-remotely:
>>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>>
>>>> type: boolean
>>>
>>> Boolean comes under flag in types.yaml
>>>
>>> definitions:
>>>   flag:
>>>     oneOf:
>>>       - type: boolean
>>>         const: true
>>>       - type: 'null'
>>>
>>> I have seen other boolean properties(spi-cpol, spi-cpha and bunch of
>>> others) using type flag. I think we should keep flag here.
>>
>> type:boolean is just shorter and example-schema recommends it. If you
>> want to base on something (as a template, pattern) then the
>> example-schema is the source, the preferred one.
> 
> I had seen other spec using flag and that's why kept same here.

I understand, you mentioned it before. However other spec is not the
example-schema...

> Which example schema are you talking about?

There is only one example-schema.
$ find ./linux -name 'example-schema*'

>>>> clocks, clock-names, qcom-ee - these are required according to old bindings.
>>>
>>> I missed qcom,ee. Will add in v3.
>>>
>>> For clocks and clock-names , there are two platforms(msm8996.dtsi,
>>> sdm845.dtsi) where these properties are missing. And I don't want to add
>>> some random values. Shall I skip them here? and let board owners add
>>> them later.
>>
>> These are required, so the SoC DTSI should be fixed. Not with random
>> clocks but something proper. :)
> 
> Yes absolutely :-)
> I have kept Srinivas in copy, who sent initial support for both the
> dtsi. Probably he can confirm provided his email doesn't bounce.
> 
> Anyway Krzysztof, can you confirm the same as you have been actively
> contributing to Qcom peripherals. I will add credit in follow-up
> submission.

Honestly not now, because I don't have access to related datasheets (I
am working on this). You can though try to look at original (vendor)
sources:
https://git.codelinaro.org/clo/la/kernel/msm-4.19 (sdm845)
https://git.codelinaro.org/clo/la/kernel/msm-3.18 (msm8996)


Best regards,
Krzysztof

> > Which example schema are you talking about?
> 
> There is only one example-schema.
> $ find ./linux -name 'example-schema*'

Example seems good to me. I will change to boolean.

> > Anyway Krzysztof, can you confirm the same as you have been actively
> > contributing to Qcom peripherals. I will add credit in follow-up
> > submission.
> 
> Honestly not now, because I don't have access to related datasheets (I
> am working on this).

Yes definitely and you also must be having bunch of items in your todo list.
Actually, I also don't have access to datasheets that's why was looking
for inputs.

> You can though try to look at original (vendor) sources:
> https://git.codelinaro.org/clo/la/kernel/msm-4.19 (sdm845)
> https://git.codelinaro.org/clo/la/kernel/msm-3.18 (msm8996)

Great. I'll see if I can make most out of it.

> > You can though try to look at original (vendor) sources:
> > https://git.codelinaro.org/clo/la/kernel/msm-4.19 (sdm845)
> > https://git.codelinaro.org/clo/la/kernel/msm-3.18 (msm8996)

I gave a look at this and couldn't find much info related to these
platforms. And waited for sometime to get reply from Srinivas and other
co.

I don't think it's viable to wait just for this particular thing and
also doesn't make much sense either. I will send next version as per
your current comments. Thanks!

Hi Kuldeep,

On Sun, 10 Apr 2022 at 23:21, Kuldeep Singh <singh.kuldeep87k@gmail.com> wrote:
>
> Convert Qualcomm BAM DMA controller binding to DT schema format using
> json schema.

Please see <https://lore.kernel.org/lkml/20220211214941.f55q5yksittut3ep@amazon.com/T/#m6700c2695ee78e79060ac338d208ffd08ac39592>,
I already have an effort ongoing for converting qcom bam DMA bindings
to YAML format.

I will send a new version of the same shortly. Please try and use the same.

Thanks,
Bhupesh

> Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com>
> ---
>  .../devicetree/bindings/dma/qcom,bam-dma.yaml | 94 +++++++++++++++++++
>  .../devicetree/bindings/dma/qcom_bam_dma.txt  | 52 ----------
>  2 files changed, 94 insertions(+), 52 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
>  delete mode 100644 Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
>
> diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> new file mode 100644
> index 000000000000..b32175d54dca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dma/qcom,bam-dma.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies Inc BAM DMA controller
> +
> +maintainers:
> +  - Andy Gross <agross@kernel.org>
> +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> +
> +allOf:
> +  - $ref: "dma-controller.yaml#"
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,bam-v1.3.0
> +      - qcom,bam-v1.4.0
> +      - qcom,bam-v1.7.0
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: bam_clk
> +
> +  "#dma-cells":
> +    const: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  iommus:
> +    minItems: 1
> +    maxItems: 4
> +
> +  num-channels:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Indicates supported number of DMA channels in a remotely controlled bam.
> +
> +  qcom,controlled-remotely:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates that the bam is controlled by remote proccessor i.e. execution
> +      environment.
> +
> +  qcom,ee:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Indicates the active Execution Environment identifier (0-7) used in the
> +      secure world.
> +
> +  qcom,num-ees:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Indicates supported number of Execution Environments in a remotely
> +      controlled bam.
> +
> +  qcom,powered-remotely:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates that the bam is powered up by a remote processor but must be
> +      initialized by the local processor.
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - "#dma-cells"
> +  - interrupts
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,gcc-msm8974.h>
> +
> +    dma-controller@f9944000 {
> +        compatible = "qcom,bam-v1.4.0";
> +        reg = <0xf9944000 0x15000>;
> +        interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&gcc GCC_BLSP2_AHB_CLK>;
> +        clock-names = "bam_clk";
> +        #dma-cells = <1>;
> +        qcom,ee = <0>;
> +    };
> +...
> diff --git a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> deleted file mode 100644
> index 6e9a5497b3f2..000000000000
> --- a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
> +++ /dev/null
> @@ -1,52 +0,0 @@
> -QCOM BAM DMA controller
> -
> -Required properties:
> -- compatible: must be one of the following:
> - * "qcom,bam-v1.4.0" for MSM8974, APQ8074 and APQ8084
> - * "qcom,bam-v1.3.0" for APQ8064, IPQ8064 and MSM8960
> - * "qcom,bam-v1.7.0" for MSM8916
> -- reg: Address range for DMA registers
> -- interrupts: Should contain the one interrupt shared by all channels
> -- #dma-cells: must be <1>, the cell in the dmas property of the client device
> -  represents the channel number
> -- clocks: required clock
> -- clock-names: must contain "bam_clk" entry
> -- qcom,ee : indicates the active Execution Environment identifier (0-7) used in
> -  the secure world.
> -- qcom,controlled-remotely : optional, indicates that the bam is controlled by
> -  remote proccessor i.e. execution environment.
> -- qcom,powered-remotely : optional, indicates that the bam is powered up by
> -  a remote processor but must be initialized by the local processor.
> -- num-channels : optional, indicates supported number of DMA channels in a
> -  remotely controlled bam.
> -- qcom,num-ees : optional, indicates supported number of Execution Environments
> -  in a remotely controlled bam.
> -
> -Example:
> -
> -       uart-bam: dma@f9984000 = {
> -               compatible = "qcom,bam-v1.4.0";
> -               reg = <0xf9984000 0x15000>;
> -               interrupts = <0 94 0>;
> -               clocks = <&gcc GCC_BAM_DMA_AHB_CLK>;
> -               clock-names = "bam_clk";
> -               #dma-cells = <1>;
> -               qcom,ee = <0>;
> -       };
> -
> -DMA clients must use the format described in the dma.txt file, using a two cell
> -specifier for each channel.
> -
> -Example:
> -       serial@f991e000 {
> -               compatible = "qcom,msm-uart";
> -               reg = <0xf991e000 0x1000>
> -                       <0xf9944000 0x19000>;
> -               interrupts = <0 108 0>;
> -               clocks = <&gcc GCC_BLSP1_UART2_APPS_CLK>,
> -                       <&gcc GCC_BLSP1_AHB_CLK>;
> -               clock-names = "core", "iface";
> -
> -               dmas = <&uart-bam 0>, <&uart-bam 1>;
> -               dma-names = "rx", "tx";
> -       };
> --
> 2.25.1
>

On Mon, Apr 18, 2022 at 10:57:55AM +0530, Bhupesh Sharma wrote:
> Please see <https://lore.kernel.org/lkml/20220211214941.f55q5yksittut3ep@amazon.com/T/#m6700c2695ee78e79060ac338d208ffd08ac39592>,
> I already have an effort ongoing for converting qcom bam DMA bindings
> to YAML format.

Ohh ok, I wasn't aware you had similar series.
I just noticed your latest v5 version was rolled out ~5 months back,
usually this is a very long time considering the duration. Wondering
reason behind this..

My updated series(v3 version[1]) is kind of complete and mostly reviewed
by Krzysztof and takes care of armv7/8 based platforms. With no offence,
I believe we should go with the current one as your series includes
changes more than BAM and will take long time to merge. Anyway, I'll be
fine with choice of the maintainers.

Regards
Kuldeep
[1] https://lore.kernel.org/linux-devicetree/20220417210436.6203-1-singh.kuldeep87k@gmail.com/T/#m2e1df4a579d0f40e07638e117df342b886289bb0

On 18/04/2022 21:20, Kuldeep Singh wrote:
> On Mon, Apr 18, 2022 at 10:57:55AM +0530, Bhupesh Sharma wrote:
>> Please see <https://lore.kernel.org/lkml/20220211214941.f55q5yksittut3ep@amazon.com/T/#m6700c2695ee78e79060ac338d208ffd08ac39592>,
>> I already have an effort ongoing for converting qcom bam DMA bindings
>> to YAML format.
> 
> Ohh ok, I wasn't aware you had similar series.
> I just noticed your latest v5 version was rolled out ~5 months back,
> usually this is a very long time considering the duration. Wondering
> reason behind this..
> 
> My updated series(v3 version[1]) is kind of complete and mostly reviewed
> by Krzysztof and takes care of armv7/8 based platforms. 

My review was only about patch correctness, not overall patch preference.

> With no offence,
> I believe we should go with the current one as your series includes
> changes more than BAM and will take long time to merge. Anyway, I'll be
> fine with choice of the maintainers.

I appreciate your work Kuldeep, it is important and valuable
contribution. It is sad to see duplicated effort, I don't like it for my
own patches either. In general, I believe the FIFO approach should be
applied, so in this case Bhupesh patches.

Before starting the conversion the best is to look for prior work on lore:
https://lore.kernel.org/lkml/?q=dfn%3Aqcom_bam_dma.txt
This way you could easily avoid doing the same.

Bhupesh,
Please check what was stopping your work, you might need to rebase it
and resend it.

Best regards,
Krzysztof

> I appreciate your work Kuldeep, it is important and valuable
> contribution. It is sad to see duplicated effort, I don't like it for my
> own patches either. In general, I believe the FIFO approach should be
> applied, so in this case Bhupesh patches.

Yep, I also agree with FIFO approach w.r.t contributions. But one thing
daunts me here is the waiting time with latest revision, it's too high.

Anyway, Bhupesh had more than BAM changes and was already on v5, I can
give benefit of doubt to him and won't argue much here.

Bhupesh, feel free to include my armv7 based dts patches in your series
otherwise you might stumble DT checks warnings.

On Wed, Apr 20, 2022 at 06:59:55PM +0530, Kuldeep Singh wrote:
> > I appreciate your work Kuldeep, it is important and valuable
> > contribution. It is sad to see duplicated effort, I don't like it for my
> > own patches either. In general, I believe the FIFO approach should be
> > applied, so in this case Bhupesh patches.
> 
> Yep, I also agree with FIFO approach w.r.t contributions. But one thing
> daunts me here is the waiting time with latest revision, it's too high.
> 
> Anyway, Bhupesh had more than BAM changes and was already on v5, I can
> give benefit of doubt to him and won't argue much here.
> 
> Bhupesh, feel free to include my armv7 based dts patches in your series
> otherwise you might stumble DT checks warnings.

Or do you want me to keep my changes separate? Sorry for spam.

On Wed, 20 Apr 2022 at 21:03, Kuldeep Singh <singh.kuldeep87k@gmail.com> wrote:
>
> On Wed, Apr 20, 2022 at 06:59:55PM +0530, Kuldeep Singh wrote:
> > > I appreciate your work Kuldeep, it is important and valuable
> > > contribution. It is sad to see duplicated effort, I don't like it for my
> > > own patches either. In general, I believe the FIFO approach should be
> > > applied, so in this case Bhupesh patches.
> >
> > Yep, I also agree with FIFO approach w.r.t contributions. But one thing
> > daunts me here is the waiting time with latest revision, it's too high.
> >
> > Anyway, Bhupesh had more than BAM changes and was already on v5, I can
> > give benefit of doubt to him and won't argue much here.
> >
> > Bhupesh, feel free to include my armv7 based dts patches in your series
> > otherwise you might stumble DT checks warnings.
>
> Or do you want me to keep my changes separate? Sorry for spam.

Please send your changes separately, as my patchset already exceeds 25
patches or so in the current form.

Thanks,
Bhupesh