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

[Manikanta Mylavarapu]

On 6/14/2023 7:29 PM, Krzysztof Kozlowski wrote:
> On 14/06/2023 13:43, Manikanta Mylavarapu wrote:
> > > > > > > > > > +    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
> > > > >
> > > > > What mandates that there's just one QCN6122 device attached to PCI?
> > > > > Assuming fixed PCI configurations like that makes me worried.
> > > >
> > > > IPQ5018 always has one internal radio, attached pcie radio's depends on
> > > > no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two
> > > > qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's
> > > > number of pcie devices controlled by QDSP6.
> > >
> > > So this is hot-pluggable (or at least board-pluggable), then should not
> > > be a part of static DTS.
> > >
> > > Some concepts of virtual-processes is anyway far away from hardware
> > > description, thus does not fit into DTS. Adding now to the equation PCIe
> > > with variable number of such processes, brings us even further.
> > >
> > > This is not a DT property. Remember - DT describes hardware.
> > >
> > > Best regards,
> > > Krzysztof
> >
> > In the multipd architecture based Socs, There is one Q6 DSP which runs
> > the OS/kernel and there are one or more instances of WCSS radios
> > (It can be either internal or pcie attached).
> > These WCSS cores are controlled by the Q6 (Q6 DSP brings wcss radios out
> > of reset/ shuts it down etc). Q6 forms the 'root Protection domain' and
> > the wcss radios are termed as the 'user protection domain'.
> > The compatible's that is being added here are to manage the 'root
> > domain' and 'user domain'.
> > Not sure if using the words 'pcie'/'ahb' made it confusing.
> > So, 'qcom,ipq5018-q6-mpd' and 'qcom,ipq5018-wcss-mpd'.
> >
> > There will be multiple instances of 'qcom,ipq5018-wcss-mpd' in DT based
> > on number of wcss radios connected on that board and only one instance
> > of 'qcom,ipq5018-q6-mpd'.
>
> I don't understand why the user protection domains need a specific
> compatible. Why do they need compatible at all?
>
> Not mentioning that amount of your domains on Q6 is actually fixed per
> SoC and probably should not be in DT at all.
  root domain is fixed per soc (One Q6 DSP, one per soc)
  user domain(s) are variable (based on number of wcss radios attached)

  The sequence to initialize, bring up, tear down the Q6 and wcss radios
  are completely different. So in order to differentiate them, we will
  need different compatibles. So this is a new rproc driver/architecture
  which has a parent/child topology (Q6 DSP -> Master/parent controls
  the WCSS (child)).

> Qualcomm puts so many so weird stuff into DT which is not a hardware
> description. I understand that everything is there a firmware, but then
> make it discoverable for example...

   Hmm, in order to init/bring up these domains, clks, resets
   and some more register access are required. Q6 FW does not have
   access to all these resources. Also, HLOS rproc driver is needed to
   listen on all the events related to  these domains just like any
   other rproc driver.

Regards,
Manikanta