[PATCH 2/2] firmware: arm_scmi: Add compatibility checks for shmem node

[Sudeep Holla]

(I saw your later reply but had started replying, so sending anyway.)

On Thu, Jun 03, 2021 at 10:18:20AM -0700, Florian Fainelli wrote:
> On 6/2/21 12:53 AM, Sudeep Holla wrote:
> > On Wed, Jun 02, 2021 at 09:44:40AM +0200, Etienne Carriere wrote:
> >> On Wed, 2 Jun 2021 at 09:37, Sudeep Holla <sudeep.holla at arm.com> wrote:
> >>>
> >>> On Wed, Jun 02, 2021 at 09:33:03AM +0200, Etienne Carriere wrote:
> >>>> Hello Sudeep,
> >>>>
> >>>>
> >>>> On Wed, 2 Jun 2021 at 00:51, Sudeep Holla <sudeep.holla at arm.com> wrote:
> >>>>>
> >>>>> The shared memory node used for communication between the firmware and
> >>>>> the OS should be compatible with "arm,scmi-shmem". Add the check for the
> >>>>> same while parsing the node before fetching the memory regions.
> >>>>>
> >>>>> Cc: Cristian Marussi <cristian.marussi at arm.com>
> >>>>> Cc: Florian Fainelli <f.fainelli at gmail.com>
> >>>>> Cc: Jim Quinlan <jim2101024 at gmail.com>
> >>>>> Cc: Etienne Carriere <etienne.carriere at linaro.org>
> >>>>> Signed-off-by: Sudeep Holla <sudeep.holla at arm.com>
> >>>>> ---
> >>>>>  drivers/firmware/arm_scmi/mailbox.c | 3 +++
> >>>>>  drivers/firmware/arm_scmi/smc.c     | 3 +++
> >>>>>  2 files changed, 6 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
> >>>>> index 4626404be541..e3dcb58314ae 100644
> >>>>> --- a/drivers/firmware/arm_scmi/mailbox.c
> >>>>> +++ b/drivers/firmware/arm_scmi/mailbox.c
> >>>>> @@ -69,6 +69,9 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> >>>>>                 return -ENOMEM;
> >>>>>
> >>>>>         shmem = of_parse_phandle(cdev->of_node, "shmem", idx);
> >>>>> +       if (!of_device_is_compatible(shmem, "arm,scmi-shmem"))
> >>>>> +               return -ENXIO;
> >>>>
> >>>> Before this change, one could use another type of memory node, like "mmio-sram".
> >>>> Is there a strong reason to enforce use of "arm,scmi-shmem" nodes?
> >>>>
> >>>
> >>> No that is for the entire SRAM which still holds and generic on-chip SRAM
> >>> driver will take care of that, this is only for the subsections that is
> >>> reserved for the scp shmem. The binding has been always there, just the
> >>> missing check. When I move to yaml, I realised that and hence the
> >>> addition of check.
> >>
> >> Ok, I understand. True the binding was there but only in the DTS
> >> examples snipped.
> >> This constraint on the compatible property of the shmem node should be
> >> clearly stated in the yaml I think.
> >>
> > 
> > Was this missing in your DTS files ? Just curious.
> > 
> 
> FWIW, our legacy DTs would have the following:
> 
> 	reserved-memory {
>                 /* This is a placeholder */
>                 NWMBOX: NWMBOX {
>                 };
>         };
> 
>        brcm_scmi: brcm_scmi at 0 {
>                 compatible = "arm,scmi-smc", "arm,scmi";
>                 mboxes = <&brcm_scmi_mailbox 0>, <&brcm_scmi_mailbox 1>;
>                 mbox-names = "tx", "rx";
>                 shmem = <&NWMBOX>;
>                 status = "disabled";
> 
> so while we have since switched to the SMC transport, the shared memory
> still does not have an "arm,scmi-shmem" compatible string, and this is a
> relatively new thing, so I am not sure we can enforce that just yet?

Since multiple people got the same doubt, I went and checked, it has been
there since we introduced the bindings in v4.17

https://elixir.bootlin.com/linux/v4.17/source/Documentation/devicetree/bindings/arm/arm,scmi.txt#L88

Converting to yaml made to add this check which was missing. So ideally
it is not any compatibility issues. Just that we were never good at following
the binding before 😉. Happened to me, sinve Juno upstream has old SCPI
support, I had a patch to change it to SCMI as we test many things on Juno,
and found even I didn't follow what I wrote in the binding correctly.
Thanks to YAML which found the issue, so thought of adding check in the
code too in case people ignore YAML and expect to work, we will fail.
Mission accomplished 😁!

--
Regards,
Sudeep