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@xxxxxxxxxxxx>
> > > > > > ---
> > > > > > .../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
