Re: [PATCH v1 01/15] dt-binding: remoteproc: mediatek: Support dual-core SCP

[Tinghan Shen <tinghan.shen@mediatek.com>]

Hi Krzysztof,

On Thu, 2022-06-02 at 12:37 +0200, Krzysztof Kozlowski wrote:
> On 02/06/2022 10:58, Tinghan Shen wrote:
> > Hi Krzysztof,
> > 
> > On Thu, 2022-06-02 at 08:55 +0200, Krzysztof Kozlowski wrote:
> > > On 02/06/2022 07:21, Tinghan Shen wrote:
> > > > Hi Krzysztof,
> > > > 
> > > > On Wed, 2022-06-01 at 13:50 +0200, Krzysztof Kozlowski wrote:
> > > > > On 01/06/2022 13:21, Tinghan Shen wrote:
> > > > > > The SCP co-processor is a dual-core RISC-V MCU on MT8195.
> > > > > > 
> > > > > > Add a new property to identify each core and helps to find drivers
> > > > > > through device tree API to cooperate with each other, e.g. boot flow and
> > > > > > watchdog timeout flow.
> > > > > > 
> > > > > > Add a new compatile for the driver of SCP 2nd core.
> > > > > > 
> > > > > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > > > > > ---
> > > > > >  .../devicetree/bindings/remoteproc/mtk,scp.yaml      | 12 ++++++++++++
> > > > > >  1 file changed, 12 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > > > > b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > > > > index eec3b9c4c713..b181786d9575 100644
> > > > > > --- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > > > > @@ -20,6 +20,7 @@ properties:
> > > > > >        - mediatek,mt8186-scp
> > > > > >        - mediatek,mt8192-scp
> > > > > >        - mediatek,mt8195-scp
> > > > > > +      - mediatek,mt8195-scp-dual
> > > > > >  
> > > > > >    reg:
> > > > > >      description:
> > > > > > @@ -57,6 +58,16 @@ properties:
> > > > > >    memory-region:
> > > > > >      maxItems: 1
> > > > > >  
> > > > > > +  mediatek,scp-core:
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > > > +    description:
> > > > > > +      The property value is a list with 2 items, a core id and a phandle
> > > > > 
> > > > > uint32, not phandle.
> > > > > 
> > > > > > +      to the sibling SCP node. 
> > > > > 
> > > > > Skip this. First part is obvious from the schema, second part should be
> > > > > described via items.
> > > > > 
> > > > > The core id represents the id of the dts node contains
> > > > > > +      this property. The valid values of core id are 0 and 1 for dual-core SCP.
> > > > > > +      The phandle of sibling SCP node is used to find the register settings,
> > > > > > +      trigger core dependent callback, and invoke rproc API.
> > > > > 
> > > > > Entire description did not help me to understand what's this. So far it
> > > > > looks like it is not a hardware property but some programming help, so
> > > > > it does not look like properly described in bindings.
> > > > > 
> > > > > > +    maxItems: 1
> > > > > 
> > > > > In description you said - two items.
> > > > > 
> > > > > You need allOf:if:then disallowing this property for other variants.
> > > > > 
> > > > > > +
> > > > > >  required:
> > > > > >    - compatible
> > > > > >    - reg
> > > > > > @@ -115,6 +126,7 @@ examples:
> > > > > >          reg-names = "sram", "cfg", "l1tcm";
> > > > > >          clocks = <&infracfg CLK_INFRA_SCPSYS>;
> > > > > >          clock-names = "main";
> > > > > > +        mediatek,scp-core = <0 &scp_dual>;
> > > > > 
> > > > > This looks like phandle, so wrong type.
> > > > > >  
> > > > > >          cros_ec {
> > > > > >              mediatek,rpmsg-name = "cros-ec-rpmsg";
> > > > 
> > > > Thanks for your feedback.
> > > > After looking for a comparable uses case, I find out a different approach.
> > > > 
> > > >   mediatek,scp-core:
> > > >     $ref: "/schemas/types.yaml#/definitions/phandle-array"
> > > >     description:
> > > >       Enable the dual-core support in scp driver.
> > > 
> > > You describe desired functional behavior, not the hardware. What is the
> > > property about? If you just want to indicate this is two-core processor,
> > > then it could be:
> > > 	mediatek,cores = <2>; /* number of cores */
> > > 
> > > 
> > > However it seems you want to achieve here something different and as I
> > > raised last time - it does not look like DT property.
> > > 
> > > Or maybe this is for first core and you want to indicate the sibling?
> > > Something like that was mentioned in previous description.
> > 
> > This property is mainly added for scp 1st core driver 
> > and scp 2nd core driver to find each other via DT API.
> > 
> > After reconsidering the use of core id in the scp driver, it 
> > is not necessary in the control flow. I'll remove the core id 
> > at next version.
> > 
> > How about change the description as following,
> > 
> >   This property enables the dual-core support in scp driver.
> >   By providing the phandle of SCP 2nd core node, the 1st SCP node
> >   can control the SCP 2nd core as the subdevice of remoteproc framework.
> 
> Please, read it again:
> 
> > > You describe desired functional behavior, not the hardware.
> 
> Again, you describe Linux implementation (scp driver, remoteproc
> framework). You need to describe the hardware, not Linux drivers.
> 
> Maybe the hardware property is that one core has its sibling and you
> provide here that sibling?

Yes, exactly. I'm sorry for misreading your argument.

How about this one,
   
  Reference to the sibling SCP core. This is required when 
  dual-core SCP support is enabled.

Thanks,
TingHan