| Message ID | 20210601225125.918225-2-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] firmware: arm_scpi: Add compatibility checks for shmem node | expand |
On Tue, Jun 01, 2021 at 11:51:25PM +0100, Sudeep Holla 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. > Ignore this patch, sent a wrong version. This breaks build... > Cc: Cristian Marussi <cristian.marussi@arm.com> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Jim Quinlan <jim2101024@gmail.com> > Cc: Etienne Carriere <etienne.carriere@linaro.org> > Signed-off-by: Sudeep Holla <sudeep.holla@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; > + > ret = of_address_to_resource(shmem, 0, &res); > of_node_put(shmem); > if (ret) { > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c > index fcbe2677f84b..78198ef94438 100644 > --- a/drivers/firmware/arm_scmi/smc.c > +++ b/drivers/firmware/arm_scmi/smc.c > @@ -76,6 +76,9 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, > return -ENOMEM; > > np = of_parse_phandle(cdev->of_node, "shmem", 0); > + if (!of_device_is_compatible(shmem, "arm,scmi-shmem")) s/shmem/np/ > + return -ENXIO; > + > ret = of_address_to_resource(np, 0, &res); > of_node_put(np); > if (ret) { > -- > 2.25.1 >
Hello Sudeep, On Wed, 2 Jun 2021 at 00:51, Sudeep Holla <sudeep.holla@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@arm.com> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Jim Quinlan <jim2101024@gmail.com> > Cc: Etienne Carriere <etienne.carriere@linaro.org> > Signed-off-by: Sudeep Holla <sudeep.holla@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? Regards, Etienne > + > ret = of_address_to_resource(shmem, 0, &res); > of_node_put(shmem); > if (ret) { > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c > index fcbe2677f84b..78198ef94438 100644 > --- a/drivers/firmware/arm_scmi/smc.c > +++ b/drivers/firmware/arm_scmi/smc.c > @@ -76,6 +76,9 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, > return -ENOMEM; > > np = of_parse_phandle(cdev->of_node, "shmem", 0); > + if (!of_device_is_compatible(shmem, "arm,scmi-shmem")) > + return -ENXIO; > + > ret = of_address_to_resource(np, 0, &res); > of_node_put(np); > if (ret) { > -- > 2.25.1 >
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@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@arm.com> > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > Cc: Jim Quinlan <jim2101024@gmail.com> > > Cc: Etienne Carriere <etienne.carriere@linaro.org> > > Signed-off-by: Sudeep Holla <sudeep.holla@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.
On Wed, 2 Jun 2021 at 09:37, Sudeep Holla <sudeep.holla@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@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@arm.com> > > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > > Cc: Jim Quinlan <jim2101024@gmail.com> > > > Cc: Etienne Carriere <etienne.carriere@linaro.org> > > > Signed-off-by: Sudeep Holla <sudeep.holla@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. br, etienne > > -- > Regards, > Sudeep
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@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@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@arm.com> > > > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > > > Cc: Jim Quinlan <jim2101024@gmail.com> > > > > Cc: Etienne Carriere <etienne.carriere@linaro.org> > > > > Signed-off-by: Sudeep Holla <sudeep.holla@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.
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@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@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@arm.com> >>>>> Cc: Florian Fainelli <f.fainelli@gmail.com> >>>>> Cc: Jim Quinlan <jim2101024@gmail.com> >>>>> Cc: Etienne Carriere <etienne.carriere@linaro.org> >>>>> Signed-off-by: Sudeep Holla <sudeep.holla@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@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?
On 6/3/21 10:18 AM, 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@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@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@arm.com> >>>>>> Cc: Florian Fainelli <f.fainelli@gmail.com> >>>>>> Cc: Jim Quinlan <jim2101024@gmail.com> >>>>>> Cc: Etienne Carriere <etienne.carriere@linaro.org> >>>>>> Signed-off-by: Sudeep Holla <sudeep.holla@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@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? Sorry, I incorrectly browsed the binding history, this is not a new requirement, will make sure we fix it at our end, too.
(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@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@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@arm.com> > >>>>> Cc: Florian Fainelli <f.fainelli@gmail.com> > >>>>> Cc: Jim Quinlan <jim2101024@gmail.com> > >>>>> Cc: Etienne Carriere <etienne.carriere@linaro.org> > >>>>> Signed-off-by: Sudeep Holla <sudeep.holla@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@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
On Thu, Jun 03, 2021 at 10:20:32AM -0700, Florian Fainelli wrote: > On 6/3/21 10:18 AM, 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@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@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@arm.com> > >>>>>> Cc: Florian Fainelli <f.fainelli@gmail.com> > >>>>>> Cc: Jim Quinlan <jim2101024@gmail.com> > >>>>>> Cc: Etienne Carriere <etienne.carriere@linaro.org> > >>>>>> Signed-off-by: Sudeep Holla <sudeep.holla@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@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? > > Sorry, I incorrectly browsed the binding history, this is not a new > requirement, will make sure we fix it at our end, too. Indeed, I cross checked that when I hit the issue on Juno yesterday. Anyways this version has build issues, was sent by mistake. I have resend the correct ones later[1]
On Wed, 2 Jun 2021 at 09:53, Sudeep Holla <sudeep.holla@arm.com> 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@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@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@arm.com> > > > > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > > > > Cc: Jim Quinlan <jim2101024@gmail.com> > > > > > Cc: Etienne Carriere <etienne.carriere@linaro.org> > > > > > Signed-off-by: Sudeep Holla <sudeep.holla@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. Yes it was :( > > -- > Regards, > Sudeep
On Thu, Jun 03, 2021 at 08:19:26PM +0200, Etienne Carriere wrote: > On Wed, 2 Jun 2021 at 09:53, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > Was this missing in your DTS files ? Just curious. > > Yes it was :( > Oh well, not a surprise as I missed it too on Juno! Hopefully YAML schema must catch it in future. -- Regards, Sudeep
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; + ret = of_address_to_resource(shmem, 0, &res); of_node_put(shmem); if (ret) { diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c index fcbe2677f84b..78198ef94438 100644 --- a/drivers/firmware/arm_scmi/smc.c +++ b/drivers/firmware/arm_scmi/smc.c @@ -76,6 +76,9 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, return -ENOMEM; np = of_parse_phandle(cdev->of_node, "shmem", 0); + if (!of_device_is_compatible(shmem, "arm,scmi-shmem")) + return -ENXIO; + ret = of_address_to_resource(np, 0, &res); of_node_put(np); if (ret) {
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@arm.com> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Jim Quinlan <jim2101024@gmail.com> Cc: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_scmi/mailbox.c | 3 +++ drivers/firmware/arm_scmi/smc.c | 3 +++ 2 files changed, 6 insertions(+)