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

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

[Sudeep Holla]

The shared memory node used for communication between the firmware and
the OS should be compatible with one of the following:
	- amlogic,meson-gxbb-scp-shmem
	- amlogic,meson-axg-scp-shmem
	- arm,juno-scp-shmem
	- arm,scp-shmem
Add the check for the same while parsing the node before fetching the memory
regions.

Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scpi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index d0dee37ad522..0fb4fe53523d 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -897,6 +897,13 @@ static const struct of_device_id legacy_scpi_of_match[] = {
 	{},
 };
 
+static const struct of_device_id shmem_of_match[] = {
+	{ .compatible = "amlogic,meson-gxbb-scp-shmem", },
+	{ .compatible = "amlogic,meson-axg-scp-shmem", },
+	{ .compatible = "arm,juno-scp-shmem", },
+	{ .compatible = "arm,scp-shmem", },
+	{ }
+};
 static int scpi_probe(struct platform_device *pdev)
 {
 	int count, idx, ret;
@@ -933,6 +940,9 @@ static int scpi_probe(struct platform_device *pdev)
 		struct mbox_client *cl = &pchan->cl;
 		struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
 
+		if (!of_match_device(shmem_of_match, shmem))
+			return -ENXIO;
+
 		ret = of_address_to_resource(shmem, 0, &res);
 		of_node_put(shmem);
 		if (ret) {
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

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;
+
 	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] firmware: arm_scpi: Add compatibility checks for shmem node

[Sudeep Holla]

On Tue, Jun 01, 2021 at 11:51:24PM +0100, Sudeep Holla wrote:
> The shared memory node used for communication between the firmware and
> the OS should be compatible with one of the following:
> 	- amlogic,meson-gxbb-scp-shmem
> 	- amlogic,meson-axg-scp-shmem
> 	- arm,juno-scp-shmem
> 	- arm,scp-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: Kevin Hilman <khilman@baylibre.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scpi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index d0dee37ad522..0fb4fe53523d 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -897,6 +897,13 @@ static const struct of_device_id legacy_scpi_of_match[] = {
>  	{},
>  };
>  
> +static const struct of_device_id shmem_of_match[] = {
> +	{ .compatible = "amlogic,meson-gxbb-scp-shmem", },
> +	{ .compatible = "amlogic,meson-axg-scp-shmem", },
> +	{ .compatible = "arm,juno-scp-shmem", },
> +	{ .compatible = "arm,scp-shmem", },
> +	{ }
> +};
>  static int scpi_probe(struct platform_device *pdev)
>  {
>  	int count, idx, ret;
> @@ -933,6 +940,9 @@ static int scpi_probe(struct platform_device *pdev)
>  		struct mbox_client *cl = &pchan->cl;
>  		struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
>  
> +		if (!of_match_device(shmem_of_match, shmem))

This must be of_match_node

> +			return -ENXIO;
> +
>  		ret = of_address_to_resource(shmem, 0, &res);
>  		of_node_put(shmem);
>  		if (ret) {
> -- 
> 2.25.1
> 

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

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
> 

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Etienne Carriere]

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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Etienne Carriere]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Florian Fainelli]

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?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Florian Fainelli]

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.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@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 😉. 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

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]

-- 
Regards,
Sudeep

[1] https://lore.kernel.org/r/20210602073851.1005607-2-sudeep.holla@arm.com/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Etienne Carriere]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel