Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox

[Jassi Brar]

On Mon, Sep 9, 2019 at 8:32 AM Andre Przywara <andre.przywara@xxxxxxx> wrote:
>
> On Fri, 30 Aug 2019 03:12:29 -0500
> Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote:
>
> Hi,
>
> > On Fri, Aug 30, 2019 at 3:07 AM Peng Fan <peng.fan@xxxxxxx> wrote:
> > >
> > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM
> > > > SMC/HVC mailbox
> > > >
> > > > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan <peng.fan@xxxxxxx> wrote:
> > > > >
> > > > > Hi Jassi,
> > > > >
> > > > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc
> > > > > > for the ARM SMC/HVC mailbox
> > > > > >
> > > > > > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan <peng.fan@xxxxxxx> wrote:
> > > > > >
> > > > > > > > > +examples:
> > > > > > > > > +  - |
> > > > > > > > > +    sram@910000 {
> > > > > > > > > +      compatible = "mmio-sram";
> > > > > > > > > +      reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > > > > > +      #address-cells = <1>;
> > > > > > > > > +      #size-cells = <1>;
> > > > > > > > > +      ranges = <0 0x0 0x93f000 0x1000>;
> > > > > > > > > +
> > > > > > > > > +      cpu_scp_lpri: scp-shmem@0 {
> > > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > > +        reg = <0x0 0x200>;
> > > > > > > > > +      };
> > > > > > > > > +
> > > > > > > > > +      cpu_scp_hpri: scp-shmem@200 {
> > > > > > > > > +        compatible = "arm,scmi-shmem";
> > > > > > > > > +        reg = <0x200 0x200>;
> > > > > > > > > +      };
> > > > > > > > > +    };
> > > > > > > > > +
> > > > > > > > > +    firmware {
> > > > > > > > > +      smc_mbox: mailbox {
> > > > > > > > > +        #mbox-cells = <1>;
> > > > > > > > > +        compatible = "arm,smc-mbox";
> > > > > > > > > +        method = "smc";
> > > > > > > > > +        arm,num-chans = <0x2>;
> > > > > > > > > +        transports = "mem";
> > > > > > > > > +        /* Optional */
> > > > > > > > > +        arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > > > > > > > >
> > > > > > > > SMC/HVC is synchronously(block) running in "secure mode", i.e,
> > > > > > > > there can only be one instance running platform wide. Right?
> > > > > > >
> > > > > > > I think there could be channel for TEE, and channel for Linux.
> > > > > > > For virtualization case, there could be dedicated channel for each VM.
> > > > > > >
> > > > > > I am talking from Linux pov. Functions 0xfe and 0xff above, can't
> > > > > > both be active at the same time, right?
> > > > >
> > > > > If I get your point correctly,
> > > > > On UP, both could not be active. On SMP, tx/rx could be both active,
> > > > > anyway this depends on secure firmware and Linux firmware design.
> > > > >
> > > > > Do you have any suggestions about arm,func-ids here?
> > > > >
> > > > I was thinking if this is just an instruction, why can't each channel be
> > > > represented as a controller, i.e, have exactly one func-id per controller node.
> > > > Define as many controllers as you need channels ?
> > >
> > > I am ok, this could make driver code simpler. Something as below?
> > >
> > >     smc_tx_mbox: tx_mbox {
> > >       #mbox-cells = <0>;
> > >       compatible = "arm,smc-mbox";
> > >       method = "smc";
> > >       transports = "mem";
> > >       arm,func-id = <0xc20000fe>;
> > >     };
> > >
> > >     smc_rx_mbox: rx_mbox {
> > >       #mbox-cells = <0>;
> > >       compatible = "arm,smc-mbox";
> > >       method = "smc";
> > >       transports = "mem";
> > >       arm,func-id = <0xc20000ff>;
> > >     };
> > >
> > >     firmware {
> > >       scmi {
> > >         compatible = "arm,scmi";
> > >         mboxes = <&smc_tx_mbox>, <&smc_rx_mbox 1>;
> > >         mbox-names = "tx", "rx";
> > >         shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
> > >       };
> > >     };
> > >
> > Yes, the channel part is good.
> > But I am not convinced by the need to have SCMI specific "transport" mode.
>
> Why would this be SCMI specific and what is the problem with having this property?
> By the very nature of the SMC/HVC call you would expect to also pass parameters in registers.
> However this limits the amount of data you can push, so the option of reverting to a
> memory based payload sounds very reasonable.
>
Of course, it is very legit to pass data via mem and many platforms do
that. But as you note in your next post, the 'transport' doesn't seem
necessary doing what it does in the driver.

Cheers!