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