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

[Manikanta Mylavarapu]

On 6/6/2023 11:44 AM, Krzysztof Kozlowski wrote:
> On 05/06/2023 14:02, Manikanta Mylavarapu wrote:
> > > > +  memory-region:
> > > > +    items:
> > > > +      - description: Q6 pd reserved region
> > > > +
> > > > +  glink-edge:
> > > > +    $ref: /schemas/remoteproc/qcom,glink-edge.yaml#
> > > > +    description:
> > > > +      Qualcomm G-Link subnode which represents communication edge, channels
> > > > +      and devices related to the Modem.
> > > > +
> > > > +patternProperties:
> > > > +  "^pd-1|pd-2|pd-3":
> > > > +    type: object
> > > > +    description:
> > > > +      In Multipd model, WCSS pd depends on Q6 pd i.e Q6 pd should be up before
> > > > +      WCSS. It can be achieved by keeping wcss pd node as subnode of Q6
> > > > +      device node.
> > >
> > > That's not enough. Your description does not say what is this, why you
> > > have two protection domains for same compatible. What's more, it a bit
> > > deviates from hardware description.
> > WCSS means 'wireless connectivity sub system', in simple words it's a
> > wifi radio block.
> >
> > IPQ5018 SOC has both internal (AHB) wifi radio/WCSS and external (PCIE)
> > wifi radio/WCSS. In Q6, Root protection domain will provide services to
> > both internal (AHB) and external (PCIE) wifi radio's protection domain.
> > So we have two protection domains for IPQ5018, one is for internal(AHB)
> > and other is for external(PCIE) wifi radio.
>
> So it is now in email, but not in the code...
> >
I will add this info, corresponding block diagram in driver code.

> > > > +
> > > > +    properties:
> > > > +      compatible:
> > > > +        enum:
> > > > +          - qcom,ipq5018-wcss-ahb-mpd
> > > > +          - qcom,ipq9574-wcss-ahb-mpd
> > > > +          - qcom,ipq5018-wcss-pcie-mpd
> > >
> > > Keep rather alphabetical order (so both 5018 together).
> > >
> > > I also do not understand these at all. Why adding bus type to
> > > compatible? This rarely is allowed (unless it is PCIe controller within
> > > soc).
> > IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
> > external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE
> > radio's properties, i have added bus type to compatible.
>
> It's the same device - WCSS - right? We do not create multiple nodes and
> compatibles for the same devices. Bus suffixes are almost never parts of
> compatibles.
No it's not the same device. WCSS on inside IPQ5018 and WCSS attached 
via pcie to IPQ5018. Here QDSP6 managing both WCSS's.

So for better clarity i will use attached SOC ID in compatible.
Below are the new compatible's.

- qcom,ipq5018-wcss-mpd //IPQ5018 internal radio
- qcom,ipq9574-wcss-mpd	//IPQ9574 internal radio
- qcom,qcn6122-wcss-mpd //IPQ5018 attached radio

> > > Drop.
> > >
> > > > +
> > > > +unevaluatedProperties: false
> > >
> > > This changed... why?
> > 'unevaluatedProperties' is similar to 'additionalProperties' except
> > that it recognize properties declared in subschemas as well.
>
> You don't have to explain me what are unevaluatedProperties or
> additionalProperties. Let's assume that I know them. What you should
> explain is why you changed it. Where is the reference to other schema?
I will go with previously used 'additionalProperties' itself and add
'unevaluatedProperties' in glink-edge.

Thanks & Regards,
Manikanta.
> Best regards,
> Krzysztof